Skip to content

MAINT: No need to check for check for FPEs in casts to/from object #28358

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

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

seberg
Copy link
Member

@seberg seberg commented Feb 19, 2025

Since these go via Python (in some form) and Python doesn't use FPEs we can be sure that we don't need to check for FPEs.

Note that while it hides almost always spurious FPEs seen on some platforms, there could be certain chains or multiple cast situations where FPEs are checked for other reasons and the spurious FPE will show up.

So it "somewhat":

Closes gh-28351

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Feb 19, 2025
@ngoldbaum
Copy link
Member

I’ll try to see if this fixes the test on my FreeBSD VM tomorrow since it looks like we’re out of compute credits.

@seberg
Copy link
Member Author

seberg commented Feb 20, 2025

Ping @rgommers I think we have been out of credits for a little bit, could you recharge it some time?

@rgommers
Copy link
Member

Ping @rgommers I think we have been out of credits for a little bit, could you recharge it some time?

Done, sorry about that. Added 200 credits, that should last us 4-5 months at the current rate.

@seberg
Copy link
Member Author

seberg commented Feb 20, 2025

@rgommers thanks!

@ngoldbaum FWIW, I got the wrong place there (both places is fine) and CI is running now, so no need to check locally.
(I.e. unintentionally confirmed CI picks up the issue, with the previous run.)

@ngoldbaum
Copy link
Member

I was actually in the middle of building NumPy in my VM to understand why the tests failed :)

Thanks for following up. Assuming this fixes the tests let's close the other PR skipping the test on freebsd and merge this.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

I'd tend to add comments about why the flags are added, since the PR text itself makes obvious it is not obvious

EDIT: Just to be sure: I meant add comments to the changed lines.

Since these go via Python (in some form) and Python doesn't use FPEs
we can be sure that we don't need to check for FPEs.

Note that while it hides *almost always* spurious FPEs seen on some
platforms, there could be certain chains or multiple cast situations
where FPEs are checked for other reasons and the spurious FPE will
show up.

So it "somewhat":

Closes numpygh-28351
I don't think it needs a comment that we can do this, but maybe it
is nice to say that there was a reason for it.
@seberg seberg force-pushed the no-fpes-in-obj-casts branch from ee5a6c2 to f3989bf Compare February 21, 2025 08:38
@charris charris merged commit dc8f46d into numpy:main Feb 21, 2025
68 checks passed
@charris
Copy link
Member

charris commented Feb 21, 2025

Thanks Sebastian.

charris pushed a commit to charris/numpy that referenced this pull request Feb 21, 2025
…umpy#28358)

* MAINT: No need to check for check for FPEs in casts to/from object

Since these go via Python (in some form) and Python doesn't use FPEs
we can be sure that we don't need to check for FPEs.

Note that while it hides *almost always* spurious FPEs seen on some
platforms, there could be certain chains or multiple cast situations
where FPEs are checked for other reasons and the spurious FPE will
show up.

So it "somewhat":

Closes numpygh-28351

* MAINT: Follow-up, got the wrong place (the other is OK).

* DOC: Add a small comment as per review request

I don't think it needs a comment that we can do this, but maybe it
is nice to say that there was a reason for it.
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: FreeBSD fails a conversion test
5 participants