-
Notifications
You must be signed in to change notification settings - Fork 169
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-3730: Re-enable saving of blot models during outlier detection #8758
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8758 +/- ##
==========================================
+ Coverage 60.83% 60.84% +0.01%
==========================================
Files 373 373
Lines 38636 38650 +14
==========================================
+ Hits 23503 23516 +13
- Misses 15133 15134 +1 ☔ View full report in Codecov by Sentry. |
Regression test failures match the 4 failed tests from last night's run on main, so they aren't caused by this PR. |
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.
I think this looks right, but handling needs to be added to the spectral mode as well, since resampling is also done there. I can test with real data when that's done.
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.
These changes look good to me.
Testing on some spectral data I have around (jw02361-o010_20240207t162649_spec3_00001_asn.json), the blot files look like they are written out appropriately.
However, thinking about it some more, I think we should really just save the SCI array instead of copying the entire input model and setting DQ and ERR to zero. As is, the variance arrays are included but left unchanged, and there are several other unrelated arrays that don't need to be in the blot product (flatfield, pathloss, photom, etc.).
Can we change this to make the blot product closer to the median and outlier_s2d products, and just write out the SCI array?
@melanieclarke the most recent change to the |
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.
Okay, everything looks good to me. Testing again on my spectral data set, only the sci array is written and metadata is updated, save location and file names look right. I also checked a random imaging data set from the regtest data and that looks right too. Thanks!
Resolves JP-3730
Closes #8750
Recent work refactoring the memory management within the outlier detection step had disabled the creation of blot files as optional intermediate-stage outputs. This PR reverts this removal, allowing these blot models to be saved when
save_intermediate_results
is True.Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR