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

BUG: masked array division should ignore all FPEs in mask calculation #26135

Merged
merged 2 commits into from Apr 3, 2024

Conversation

carlosilva10260
Copy link
Contributor

This PR fixes the bug #25810 where an internal multiplication underflow occured with np.seterr(under="raise")

@carlosilva10260 carlosilva10260 force-pushed the linspace-speedups branch 2 times, most recently from fc2111f to c30dd94 Compare March 26, 2024 00:00
@@ -2581,6 +2581,14 @@ def test_no_masked_nan_warnings(self):
# also check that allclose uses ma ufuncs, to avoid warning
allclose(m, 0.5)

def test_masked_array_underflow(self):
np.seterr(under="raise")
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure to use with np.errstate(), this will otherwise affect later tests randomly. Otherwise, LGTM, no need to insert those newlines, but they seem fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I'll send right away the new PR

@carlosilva10260
Copy link
Contributor Author

Hi @seberg , I've fixed the test as you requested and also removed the extra spaces that I forgot to remove before.
Is everything now correct to you?

X = np.ma.array(x)
x2 = x/2.0 # <- works
X2 = X/2.0
assert(X2.all() == X.all() / 2.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this assert test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose is to make sure that the division occured fine without the multiply underflow error

@@ -2581,6 +2581,14 @@ def test_no_masked_nan_warnings(self):
# also check that allclose uses ma ufuncs, to avoid warning
allclose(m, 0.5)

def test_masked_array_underflow(self):
with(np.errstate(under="raise")):
x = np.arange(0, 3, 0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this line and the next two or of the with context, as the test is about the masked array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i'll do so

X = np.ma.array(x)
with(np.errstate(under="raise")):
X2 = X/2.0
assert(X2.all() == X.all() / 2.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(X2.all() == X.all() / 2.0)
np.testing.assert_array_equal(X2, x/2)

The X.all() and X2.all() are both False. Perhaps you wanted to write assert np.all(X2==x/2.0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the correction, at the time I wasn't 100% sure how I would test the value of the array but I'll change to that as it makes more sense.

@carlosilva10260
Copy link
Contributor Author

Hi @seberg @eendebakpt, is everything correct now in the new PR?

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

numpy/ma/tests/test_core.py Outdated Show resolved Hide resolved
@seberg seberg changed the title BUG: masked array division broken with np.seterr(under=raise) #25810 BUG: masked array division should ignore all FPEs in mask calculation Apr 3, 2024
@seberg seberg merged commit 9704767 into numpy:main Apr 3, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants