Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecations, numpy 1.23 compatibility #110

Merged
merged 15 commits into from
Jul 28, 2022
Merged

Deprecations, numpy 1.23 compatibility #110

merged 15 commits into from
Jul 28, 2022

Conversation

bethac07
Copy link
Member

Now with 100% more rebases

@bethac07
Copy link
Member Author

I canNOT replicate this failing test locally. I upgraded my cython to match the build machine version (0.29.30) deleted all my .so/.cpp/.pyc files to force recompilation, made sure my numpy and scipy versions matches (1.23.1, 1.9.0rc2), and as far as I can tell those should be the only deps for cpmorphology .

@bethac07
Copy link
Member Author

bethac07 commented Jul 22, 2022

Ok, it's failing in master too, so I didn't break it, but i do not get why it is broken https:/CellProfiler/centrosome/runs/7474979979?check_suite_focus=false#step:5:35

@bethac07
Copy link
Member Author

bethac07 commented Jul 22, 2022

I'm pretty sure I see what has to be happening, and how to fix the test, but I don't know why.

Here is the code in question - the difference in the Y between the expected and the failing test is 3 vs 5, aka 2, so y_off (and then y_center has to be off by 1 before the strel multiplies y_center * 2 .

Consider the code below:

y_off = -int(np.finfo(float).eps + np.sin(angle) * length / 2)
>>> y_off
-2
>>> np.finfo(float).eps
2.220446049250313e-16
>>> np.sin(angle) * length / 2
1.9999999999999998
>>> -int(np.sin(angle)*length/2+np.finfo(float).eps)
-2
>>> -int(np.sin(angle)*length/2)
-1

So if whatever tiny number we're adding to np.sin(angle)*length is no longer overcoming the "hurdle" to -2, the behavior is explained. But why would this happen differently on different os's? Is the precision different? The value of np.finfo(float).eps ? The value of the sin?

@bethac07
Copy link
Member Author

So, yes, changing y_off = -int(np.finfo(float).eps + np.sin(angle) * length / 2) to -int(np.round(np.finfo(float).eps + np.sin(angle) * length / 2)), and just to be parallel, I rounded the x too. All tests here and in CellProfiler are now passing. But

  • Is there anything we should follow up on here, as to why this is different between our test environment and my local? Is this potentially a numpy bug?
  • There are 3 other functions in centrosome that use either np.sin or np.cos - gabor, circular average, circular hough - do we want to investigate anything at all there?

@bethac07
Copy link
Member Author

bethac07 commented Jul 22, 2022

The value and dtype of np.sin(angle) * length / 2, np.finfo(float).eps + np.sin(angle) * length / 2, and eps = np.finfo(float).eps

On my local:

1.9999999999999998 float64
2.0 float64
2.220446049250313e-16 float64

On the build system: I don't know because apparently there unlike my local machine pytest tests with print statements in them dont' seem to actually print to the console. I give up for tonight lol. AHA YOU CANNOT OUT-STUBBORN ME

1.9999999999999996 float64
1.9999999999999998 float64
2.220446049250313e-16 float64

So either np.sin, multiplication, or division work differently; that seems bad to me.

@bethac07
Copy link
Member Author

Reported as numpy/numpy#22028

@@ -464,9 +464,9 @@ def strel_line(length, angle):
Note: uses draw_line's Bresenham algorithm to select points.
"""
angle = float(angle) * np.pi / 180.0
x_off = int(np.finfo(float).eps + np.cos(angle) * length / 2)
x_off = int(np.round(np.finfo(float).eps + np.cos(angle) * length / 2))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using scipy.special.cosdg() and sindg() to work directly with degrees. I assume the tricks with eps and round() are to make sure that fun special cases like angle = 90.0 get clean 0s. cosdg() and sindg() help with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will take a look, but at least from local testing on my Mac, it still seems like we may still need to keep one or both of the eps and round tricks: the scipy functions help with 0s but not ie 0.5

>>> import numpy as np
>>> import scipy.special
>>> fn_dict = {np.sin:scipy.special.sindg,np.cos:scipy.special.cosdg,np.tan:scipy.special.tandg}
>>> for fn in fn_dict.keys():
...     for angle in range(0,181,30):
...             print(str(fn),angle,fn(angle*np.pi/180),fn_dict[fn](angle))
... 
<ufunc 'sin'> 0 0.0 0.0
<ufunc 'sin'> 30 0.49999999999999994 0.49999999999999994
<ufunc 'sin'> 60 0.8660254037844386 0.8660254037844387
<ufunc 'sin'> 90 1.0 1.0
<ufunc 'sin'> 120 0.8660254037844388 0.8660254037844387
<ufunc 'sin'> 150 0.49999999999999994 0.49999999999999994
<ufunc 'sin'> 180 1.2246467991473532e-16 -0.0
<ufunc 'cos'> 0 1.0 1.0
<ufunc 'cos'> 30 0.8660254037844387 0.8660254037844387
<ufunc 'cos'> 60 0.5000000000000001 0.49999999999999994
<ufunc 'cos'> 90 6.123233995736766e-17 -0.0
<ufunc 'cos'> 120 -0.4999999999999998 -0.49999999999999994
<ufunc 'cos'> 150 -0.8660254037844387 -0.8660254037844387
<ufunc 'cos'> 180 -1.0 -1.0
<ufunc 'tan'> 0 0.0 0.0
<ufunc 'tan'> 30 0.5773502691896256 0.5773502691896256
<ufunc 'tan'> 60 1.7320508075688767 1.7320508075688767
<ufunc 'tan'> 90 1.633123935319537e+16 inf
<ufunc 'tan'> 120 -1.7320508075688783 -1.7320508075688767
<ufunc 'tan'> 150 -0.5773502691896256 -0.5773502691896256
<ufunc 'tan'> 180 -1.2246467991473532e-16 0.0

@bethac07 bethac07 requested a review from gnodar01 July 26, 2022 17:40
@bethac07
Copy link
Member Author

@gnodar01 I think after this we're ready to tag 1.2.1 !

@gnodar01
Copy link
Contributor

There's 50 warnings when running tests, I think all of them are related to dividing by 0, and therefore producing nans. Are we okay with those, or for that matter, or we reliant on those nan values downstream?

@bethac07
Copy link
Member Author

bethac07 commented Jul 28, 2022

Yeah, I did notice that, I think we'd likely have to go through them one at a time, some of them may definitely be relying on 0s. I think most of them are also in master, so my feeling would be "definitely let's fix in the future but let's not have it hold up 1.2.1" (which is really "let's not have it hold up 4.2.2"), but happy to be overruled here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants