-
Notifications
You must be signed in to change notification settings - Fork 32
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
Convert ramp fitting Cython .pyx files into annotated .py files #232
Conversation
Codecov ReportAttention:
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. |
@@ -0,0 +1,32 @@ | |||
# cython: language_level=3str |
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.
Unfortunately, I was unable to eliminate this file because Cython's enum
structures do not have a Python syntax equivalent at this time.
bccc94e
to
4ead91f
Compare
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? |
4ead91f
to
a44f5be
Compare
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? |
of .pyx file
for ols_cas22._fit
parameters and variances in fit_ramps()
improved performance in ols_cas22_fit.py and _fit.py
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
6081bba
to
34e59dc
Compare
Is this PR still relevant? |
Ah, we lost track of this one. I'm just going to close. |
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
CHANGES.rst
(either inBug Fixes
orChanges to API
)