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

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

Closed

Conversation

WilliamJamieson
Copy link
Collaborator

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

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

Copy link

codecov bot commented Nov 17, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,32 @@
# cython: language_level=3str
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, I was unable to eliminate this file because Cython's enum structures do not have a Python syntax equivalent at this time.

@schlafly
Copy link
Collaborator

schlafly commented Jan 2, 2024

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?

@WilliamJamieson WilliamJamieson marked this pull request as ready for review January 31, 2024 15:49
@WilliamJamieson WilliamJamieson requested a review from a team as a code owner January 31, 2024 15:49
@schlafly
Copy link
Collaborator

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?

This enables cython and the C compiler to auto inline stuff for maximum
speed.
This makes it so that we get no numerical differences, it does not effect average computation time
This prevents a bounce out of cython and does not affect compute times
This significantly enhances readability of the single
pixel fitting function.
It is complicated to use and the helper function is there
to reduce that complexity.
This is done so that romancal does not break when this
gets merged
@stscieisenhamer
Copy link
Collaborator

Is this PR still relevant?

@schlafly
Copy link
Collaborator

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

@schlafly schlafly closed this Mar 21, 2024
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.

4 participants