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

Fix floating point issues #194

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Fix floating point issues #194

merged 4 commits into from
Jan 14, 2025

Conversation

farkmarnum
Copy link
Collaborator

This fixes #193 using the solution described in that issue.

I've also added a test for the case causing the error -- even though the error only occurs on some architectures, there's always a warning from numpy about the potential overflow, so the test checks for that.

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Thanks, that looks great!

@schlafly
Copy link
Collaborator

schlafly commented Jan 8, 2025

FYI, I need to diagnose the failing lon/lat tests ~now but they are unrelated to this PR.

@schlafly
Copy link
Collaborator

schlafly commented Jan 9, 2025

@farkmarnum , I resolved the issues with tests so now the tests work. Everything passes except for the -oldestdeps test, which fails.
https://github.com/spacetelescope/romanisim/actions/runs/12677867579/job/35334182610?pr=194#step:10:554
I don't immediately understand that.

@farkmarnum
Copy link
Collaborator Author

@farkmarnum , I resolved the issues with tests so now the tests work. Everything passes except for the -oldestdeps test, which fails. https://github.com/spacetelescope/romanisim/actions/runs/12677867579/job/35334182610?pr=194#step:10:554 I don't immediately understand that.

It looks like the behavior of np.nextafter changed somewhere between numpy v1.22.0 and the current version (v2.2.1), and a solution that works across both versions is to explicitly pass the dtype instead of just correctly typing the input:

Screenshot 2025-01-14 at 3 10 04 PM

I just pushed a fix to this branch.

@schlafly
Copy link
Collaborator

Great. Thanks! All tests are passing, merging.

@schlafly schlafly merged commit 9ef0301 into main Jan 14, 2025
20 checks passed
@schlafly schlafly deleted the mfarnum/fix-floating-point-issues branch January 14, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Floating point imprecision and integer overflow issues still occurring on some architectures
2 participants