JP-4324: implement multiprocessing for Adaptive Trace Model#10391
Conversation
|
This looks great! Thanks for contributing. The docs build failure is unrelated; we should have a fix for it soon. I see you're iterating with ruff to get the style checks to pass, but see also the contributing docs for instructions if you want to get pre-commit set up to run locally. We should also mention this step in the multiprocessing section of readthedocs, in docs/jwst/user_documentation/multiprocessing.rst. These changes will need also some unit tests to exercise the new multiprocessing options. Do you want to attempt those yourself? If not, I can help. |
|
I'd appreciate your help with the tests, please, mostly since I'm not familiar with the test fixtures and other test infrastructure specific to the pipeline. I could probably figure it out based on your test code but I expect you will likely be faster at this part. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10391 +/- ##
=======================================
Coverage 85.77% 85.78%
=======================================
Files 372 372
Lines 40063 40081 +18
=======================================
+ Hits 34364 34382 +18
Misses 5699 5699 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Testing locally, these changes look great. Testing on my Mac with a NIRSpec IFU file with psf_optimal=True, processing time is improved from 7m to 1m with maximum_cores = "all", and memory use is not excessive. I sent you a PR on your fork to add some tests and clean up a few software items here: |
Tests for adaptive trace multiprocessing
pllim
left a comment
There was a problem hiding this comment.
- Ken had mention before that multiprocessing can swallow up some log messages. Is this a concern here?
- Given the description that multiprocessing is only even used under certain conditions, should there be extra checks to tell user that if they set
>1cores but really they don't have to?
Not a concern here. There are no log messages internal to the code being multiprocessed.
It's not possible to tell from the input arguments if the multiprocessing will be needed or not. The decision is data-based. It does no harm to specify multiple cores anyway. |
emolter
left a comment
There was a problem hiding this comment.
Two docs nitpicks and one question
|
@mperrin - just checking in on this PR. There are a couple minor review comments to address. If you like, I can address them on your behalf so we can move forward. |
|
Hi @melanieclarke - thanks. I had meant to get to those, but have not had time yet due to other tasks and priorities. If you have spare bandwidth now then yes by all means you're welcome to go ahead. Thanks! |
0ec9bf0 to
fad920a
Compare
|
Regression tests: All passing. |
Resolves JP-4324
This PR enhances the recently-added Adaptive Trace Model step by adding multiprocessing support, similar to that used in the jump and ramp fit steps. (i.e. parameter
maximum_coreswhich can take integers, 'half', 'all', etc). Only the spline fitting part of the code becomes multi process, not the oversampling part (which is less computationally intensive in general).Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)changes/<PR#>.breaking.rstnews fragmentdocs/pageokify_regteststo update the truth files