-
-
Notifications
You must be signed in to change notification settings - Fork 4
FIX: Register Comparison Ops Promoter for only Quddtype #60
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
Conversation
|
Can you please add a regression test based on #57? |
| where registering comparison promoters for QuadPrecDType interfered | ||
| with standard NumPy operations like np.logical_or.reduce(np.arange(10.)). | ||
| """ | ||
| result = np.logical_or.reduce(np.arange(10.)) |
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 can only reproduce the bug if I start a new Python session and then run
import numpy as np
from numpy_quaddtype import QuadPrecDType
np.logical_or.reduce(np.arange(10.))How do we ensure that the regression test also starts with a clean slate and can fully control the imports?
Ideally, can you (locally) briefly revert the fix, get the regression test to fail, then reapply the patch, and then check that the regression test works? That way we'd be sure that the issue is actually gone and doesn't reappear
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.
Actually the workflow is as you import numpy, everything is normal.
Now as you import quaddtype, previously it overrides the default NumPy's rules and enforce this comparison promoter to run for all the dtypes. So it'll happen as the session imported the quaddtype and will persists the whole session until something again overrides that rule to default.
So this PR fix will make sure at the time of import, it'll not override the default rules but make a new rule that this promoter to be only called for Quaddtype and no other dtypes
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.
One problem is that if I execute
import numpy as np
np.logical_or.reduce(np.arange(10.))
from numpy_quaddtype import QuadPrecDType
np.logical_or.reduce(np.arange(10.))then everything works. So I worry that some other tests trigger the promoter rules to silently work, even though there still is the case where it would fail
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.
Ideally, can you (locally) briefly revert the fix, get the regression test to fail, then reapply the patch, and then check that the regression test works? That way we'd be sure that the issue is actually gone and doesn't reappear
Yeah I tested that, it is behaving as expected. I can comment the patch and then the workflow on this PR will all fail
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.
Ok, thanks for testing locally, then I'm happy with this test
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.
One problem is that if I execute
import numpy as np np.logical_or.reduce(np.arange(10.)) from numpy_quaddtype import QuadPrecDType np.logical_or.reduce(np.arange(10.))then everything works. So I worry that some other tests trigger the promoter rules to silently work, even though there still is the case where it would fail
Good example, and the reason is obvious when 1st time logical_or.reduce is called, numpy caches the float64 loop and reuses it at second time (you can validate this by changing the 2nd logical_or's 10. input to just 10)
As now this will require an int loop which isn't cached and quaddtype's import already contaminated the rules
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 don't think it is essential, but usually for these kinds of tests I use subprocess.call to have a clean slate.
More importantly, this indeed allows the astropy tests to pass after import QuadPrecDType, thanks!
juntyr
left a comment
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.
LGTM
closes #57