Skip to content

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

Merged
seberg merged 2 commits into
numpy:mainfrom
carlosilva10260:linspace-speedups
Apr 3, 2024
Merged

seberg merged 2 commits into
numpy:mainfrom
carlosilva10260:linspace-speedups

Conversation

@carlosilva10260

Copy link
Copy Markdown
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
Comment thread numpy/ma/tests/test_core.py Outdated
allclose(m, 0.5)

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

Comment thread numpy/ma/tests/test_core.py Outdated
X = np.ma.array(x)
x2 = x/2.0 # <- works
X2 = X/2.0
assert(X2.all() == X.all() / 2.0)

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread numpy/ma/tests/test_core.py Outdated

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

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread numpy/ma/tests/test_core.py Outdated
X = np.ma.array(x)
with(np.errstate(under="raise")):
X2 = X/2.0
assert(X2.all() == X.all() / 2.0)

Copy link
Copy Markdown
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)?

@carlosilva10260 carlosilva10260 Mar 29, 2024 •

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@seberg seberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks.

Comment thread numpy/ma/tests/test_core.py Outdated
@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
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.

3 participants