spacetelescope / stcal

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

Convert ramp fitting Cython .pyx files into annotated .py files #232

Closed WilliamJamieson closed 7 months ago

WilliamJamieson commented 11 months ago

Cython 3+ supports compiling directly from annotated .py files instead of its own .pyx files. Indeed, Cython recommends doing this if there is no need to interface directly into C files, which is exactly the case for ramp fitting. This is because it enables the full suite of standard python tooling without the need to seek out tooling which specifically supports Cython. Moreover, it also reduces human context switching when reading the code as the "Cython" code now reads exactly like (verbose) python code.

In addition to this switch, I made several other small tweaks to ramp fitting which operationally make no difference (the code exactly reproduces the previous version's results). These tweaks net a modest ~20%-30% speed up over the current ramp fitting code. Other changes include some refactoring to make parts of the code more readable.

Note that when consulting the Cython docs, I discovered that code tends to be faster if all of it is contained within a single Cython module as the Cython C-transpiler can perform some optimizations such as inlining (i.e. Cython cannot inline a function, even if it is marked to be inlined, if the function is called from a different module). This makes the code slightly less readable but it did result in a small but noticeable performance gain.

Checklist

codecov[bot] commented 11 months ago

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (fd2d6ce) 85.98% compared to head (34e59dc) 86.00%.

Files Patch % Lines
src/stcal/ramp_fitting/ols_cas22.py 81.81% 12 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #232 +/- ## ========================================== + Coverage 85.98% 86.00% +0.01% ========================================== Files 35 36 +1 Lines 6542 6594 +52 ========================================== + Hits 5625 5671 +46 - Misses 917 923 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

schlafly commented 10 months ago

I agree it's nice to keep this "pure python" from a maintainability perspective. This exactly reproduces the previous results, so no changes to regression tests, etc., are needed?

schlafly commented 9 months ago

This code looks good to me and it's nice to be more pure-python-y. I think I understood once that this gives you literally the exact same results as before (i.e., no regression tests would need to change). Is that the case?

stscieisenhamer commented 7 months ago

Is this PR still relevant?

schlafly commented 7 months ago

Ah, we lost track of this one. I'm just going to close.