-
Notifications
You must be signed in to change notification settings - Fork 33
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 failing unit test in Python 3.13 #288
Conversation
I cherry picked e1d446d onto #287 so the wheel building would be attempted (which runs the unit tests) with python 3.13 and the change in this PR. Unfortunately the unit test failed in the same way as reported in the issue: Also, I think the CI disabled itself due to inactivity in this repo. I don't have permissions to re-enable it. Do you? |
JWST regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/13045302772 |
Unfortunately I started regression tests before reading your comment. In my local testing at some moment I switched to numpy 1.26.3 and it made no difference (test was still failing). After this fix all tests were passing. I forgot to test with numpy 2.2.0. I will investigate. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #288 +/- ##
=======================================
Coverage 80.25% 80.25%
=======================================
Files 5 5
Lines 1013 1013
=======================================
Hits 813 813
Misses 200 200 ☔ View full report in Codecov by Sentry. |
@braingram Try again. I think it should work now but I can't get regression test run past sonar scan. |
Oof, sonar scan is blocked by spacetelescope/jwst#9112 You can use the branch for that PR if you're impatient like me :) |
|
||
v = great_circle_arc.length(a, b) | ||
|
||
assert np.all(np.logical_not(np.isfinite(v))) |
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.
To check I'm understanding. Previously providing np.nan
to length
resulted in ValueError: Out of domain for acos
and with this PR it doesn't raise an exception and instead returns np.nan
?
Where is length
used and will this change in behavior have consequences?
Another attempt to run regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/13061746792 |
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 and testing locally this fixes the python 3.13 failure.
To be safe let's wait till the regtests pass then I'll update the 3.13 PR.
Thanks for finding the fix!
Regressiuon tests have numerous failures but they do not appear to be related to this PR but rather to So, ... merging this |
Fixes #285
CC: @braingram