JP-3708, JP-3771, JP-3791 #318
Conversation
…p fitting C-extension.
…e problem with segmentation faults and freezes.
…optional results product. Still need to check return value.
…reating the optional results product.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 86.38% 86.40% +0.01%
==========================================
Files 49 49
Lines 8899 8934 +35
==========================================
+ Hits 7687 7719 +32
- Misses 1212 1215 +3 ☔ View full report in Codecov by Sentry. |
kmacdonald-stsci
requested review from
melanieclarke,
tapastro,
jemorrison and
stscirij
| */ | ||
| void | ||
| set_up_logger() { | ||
| const char * log_dir = "/Users/kmacdonald/code/stcal/logs"; |
There was a problem hiding this comment.
Maybe get this location from an environment variable or some other way?
There was a problem hiding this comment.
Agreed - this needs to be removed or updated to remove the local path.
There was a problem hiding this comment.
It's a debugging function that isn't used anywhere. I left it there so I can use it in the future for debugging. I'll simply remove the path.
There was a problem hiding this comment.
Looking good!
Testing on the sample data that failed in #304, I no longer see the processing hang with the optional output + multiprocessing. Optional output looks the same with and without multiprocessing now, and also shows only minor differences from output with the main branch + algorithm = 'OLS'.
Did you run regression tests for jwst and romancal? If so, can you please link them here?
changes/318.bugfix.rst
Outdated
| @@ -0,0 +1,19 @@ | |||
| JP-3791: Fixed the `ramp fitting` multiprocessing portion of the | |||
There was a problem hiding this comment.
This note doesn't render correctly when it's added to the CHANGES.rst - it is causing errors in the docs build. I think it needs to be rephrased as a single paragraph. We also don't usually add JP ticket numbers to the change log.
| */ | ||
| void | ||
| set_up_logger() { | ||
| const char * log_dir = "/Users/kmacdonald/code/stcal/logs"; |
There was a problem hiding this comment.
Agreed - this needs to be removed or updated to remove the local path.
Here is a link to the regression tests I ran on these PRs: https://github.com/spacetelescope/RegressionTests/actions/runs/11820513473 These changes don't effect RomanCal, but I will ask a RomanCal maintainer to run regression tests against these changes. |
Resolves JP-3708
Resolves JP-3771
Resolves JP-3791
JP-3708: Add the CRMAG parameter to the optional results product in the C-extension. This wasn't implemented with the C-extension, nor was it properly handled for multiprocessing.
JP-3771: Corrected bugs exposed by adding multiprocessing regression tests to the JWST regression testing suite. The primary bug found was incorrect dimension sizes for internal arrays used for the optional results product. The internal arrays should have dimensions of
(nints, nrows, ncols), but they had dimensions(ngroups, nrows, ncols). These incorrect dimensions caused memory corruption causing processes to freeze or segfault.JP-3791: Corrected the CHARGELOSS recomputation of the read noise variance. For multiprocessing the DQ flags were not being properly sliced and passed to the C-extension for recomputation.
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change