-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix point-on-plane detection for NumPy 1.19.0 in Linux #202
base: main
Are you sure you want to change the base?
Conversation
@paulmelnikow Can you confirm that the idea behind this PR is to work around small numerical precision differences in Numpy 1.19 between your local OS X development machine, and the remote Linux CI server? |
I forget whether or not it's reproducible on OS X. Though yes, the way I'd put it is that the goal is to make the code work the same in NumPy 1.19 on Linux as it did in earlier versions of NumPy on Linux. These tests started failing when NumPy 1.19 was released and I had to pin the NumPy version back to 1.18 to get them working again. I can re-check OS X tomorrow if it's helpful. |
I am fine with this fix, but I wonder if it would be better to regard it as a workaround to a regression in numpy. Do you think this is an issue that should get reported upstream? |
@@ -1,3 +1,3 @@ | |||
numpy<1.19.0 | |||
numpy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will default to the latest published numpy. Does it make sense to tag numpy at version 1.19?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In applications I always pin NumPy for safety's sake, however in libraries, I usually don't bother, since issues like this one are fairly rare (once every couple years) and version mismatches will cause warnings downstream at install time.
In other words, it's less work to leave the NumPy version number loose.
When a library depends on a new feature in a dependency we can use >=
, which of course does not cause warnings with later versions.
I do not know for sure whether or not this is a regression. It's possible. Though in general, I do not expect floating point multiplication and division to produce values which exactly equal zero. However that's what NumPy used to do until 1.19. It's not the first time a NumPy upgrade has caused unexpected regressions in downstream code. I do know that NumPy 1.19 dropped Python 2 and a bunch of legacy code along the way. Would you like to try to isolate the issue and report it? I do think if this is a temporary workaround for a regression we should mark it up in the codebase with TODO comments. |
Let's confirm with upstream numpy that this is the expected behavior going forward, then merge this PR into our codebase. |
This fixes the tests that are failing in NumPy 1.19.0.
See https://app.circleci.com/pipelines/github/lace/polliwog/836/workflows/b88ac698-51bc-46ea-80b8-f9e57e7157d9/jobs/2618/steps
Closes #198