-
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
JP-3678: Ensure C extension works with multiprocessing #268
Conversation
… C extension gets properly set for multiprocessing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
- Coverage 83.79% 83.76% -0.03%
==========================================
Files 35 35
Lines 6998 7004 +6
==========================================
+ Hits 5864 5867 +3
- Misses 1134 1137 +3 ☔ View full report in Codecov by Sentry. |
Running a regression test: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1576/ |
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.
LG2M- confirmed based on runtime that it's now using the C algorithm when it's meant to, even with multiprocessing on.
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.
Just a few suggestions for the new log messages. Otherwise, looks good!
src/stcal/ramp_fitting/ols_fit.py
Outdated
image_info, integ_info, opt_info = ols_slope_fitter( | ||
ramp_data, gain_2d, readnoise_2d, weighting, save_opt) | ||
|
||
log.info("Returning from C extension") |
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.
log.info("Returning from C extension") | |
log.debug("Returning from C extension") |
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.
Looks good to me now. Thanks!
I forgot to mention - I looked at the regression tests that failed and they are unrelated. These three tests seem to intermittently fail fairly often (test_nirspec_missing_msa_fail, test_nirspec_missing_msa_nofail, and test_duplicate_names). |
I started another test run - the previous one did not pick up the correct commit and did not test the changes in this PR. Failures are unrelated. |
Resolves JP-3678
Closes #8618
This PR addresses an error in multiprocessing with the selection of the OLS_C algorithm. The C extension flag was not properly passed to the sliced data when prepared for multiprocessing, so the old python code ran, even the the C extension was selected. The C extension selection is now properly passed to the sliced data, so the C extension will be run when multiprocessing is selected.
Checklist
CHANGES.rst
(either inBug Fixes
orChanges to API
)