search

spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

unit test failures with python 3.13 #301

Closed braingram closed 1 day ago

braingram commented 3 weeks ago

https://github.com/spacetelescope/stcal/pull/300 adds python 3.13 to the CI and is showing failures in test_too_few_groups and test_refcounter: https://github.com/spacetelescope/stcal/actions/runs/11235389533/job/31233242973?pr=300#step:10:120

braingram commented 3 weeks ago

Assigning Ken to see if he has any ideas.

kmacdonald-stsci commented 3 weeks ago

https://github.com/spacetelescope/stcal/actions/runs/11235389533/job/31233242973?pr=300#step:10:120

Nothing immediately comes to mind. I'll take a look at this sometime today. I have a PR that changes the STCAL CI default algorithm from OLS to OLS_C. Hopefully, that will get merged soon. If there is some change in 3.13 that affects the C code, then we should expect more tests to fail.

kmacdonald-stsci commented 3 weeks ago

This is the root of the problem.

https://github.com/spacetelescope/stcal/blob/787aa81a6719bb0c72be8972171c7f3f88c8adf0/src/stcal/ramp_fitting/src/slope_fitter.c#L1833

Under the hood, this function calls PyLong_AsLong. The drop_frame1 may not be used and is passed to the C-extension as a NoneType, rather than an integer, if unused. I guess there is additional exception handling in this function with the 3.13 update.

Once https://github.com/spacetelescope/stcal/pull/298 is merged I will create another PR to add an additional check for this attribute being NoneType.

braingram commented 3 weeks ago

Thanks for the quick bug-hunting! Feel free to push to https://github.com/spacetelescope/stcal/pull/300 or include the changes in your PR so the CI can test with 3.13. Also if it's helpful to first open/review/merge that (knowing that the python 3.13 tests will fail) let me know and I'll open it for review.

kmacdonald-stsci commented 3 weeks ago

Thanks for the quick bug-hunting! Feel free to push to #300 or include the changes in your PR so the CI can test with 3.13. Also if it's helpful to first open/review/merge that (knowing that the python 3.13 tests will fail) let me know and I'll open it for review.

@braingram I opened a PR in preparation for this: https://github.com/spacetelescope/stcal/pull/303

WilliamJamieson commented 1 day ago

Looks like #303 resolved these issues.