-
Notifications
You must be signed in to change notification settings - Fork 159
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-3569: Update variance handling in resample step #8437
JP-3569: Update variance handling in resample step #8437
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8437 +/- ##
==========================================
+ Coverage 56.38% 56.42% +0.04%
==========================================
Files 387 387
Lines 38716 38754 +38
==========================================
+ Hits 21830 21868 +38
Misses 16886 16886 ☔ View full report in Codecov by Sentry. |
@melanieclarke Pro tip: If you include the name of the JP ticket in the PR title, it'll automatically create links between the two. e.g. "JP-3569: Update variance ..." |
be4e677
to
94e8555
Compare
I think this is ready for review now, if someone can start a regression test run for me. |
LG2M from the science side; thanks for adding the doc update. |
Regression tests started here: |
Reviewing the regression tests, it looks like all changes are to post-resampling products (i2d, s2d, x1d, or catalog), in the error or variance arrays only as expected:
One test result I don't understand and will look into a little closer:
The NIRISS filter rotation failure in assign_wcs looks unrelated. |
It looks like the catalog NaNs correspond to sources with saturated cores, for which the variance components are all NaN in the i2d file. The ERR is now set to NaN at these locations, instead of 0. The aperture photometry in the source_catalog does not ignore NaNs, which is why NaN values now appear in the error columns in the output catalog. They do not also appear in the flux columns because fillval=INDEF in resample, by default, so missing values are set to zero in the SCI data. If the fillval default is set to NaN, saturated stars will also report flux = NaN. @hbushouse - Should I add a mask for NaN values to the source_catalog aperture photometry, leave it as is, or file another ticket? |
Per @drlaw1558 and the JP coordination meeting today - the issue with NaN values in aperture photometry needs a separate ticket. I will leave the code as is in the catalog step. |
… error images instead of variance (not currently used)
f28e2d2
to
835da72
Compare
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, including test results and updated docs.
Resolves JP-3569
Closes #8344
Variance arrays from input data are now resampled individually, then combined with weights that match the science weight type, instead of being self-weighted. Input variance values equal to zero are propagated as zeros, rather than as NaN. In the final combined error array, values for which all input error components are NaN are set to NaN instead of zero. NaNs are otherwise ignored in the sum over variance components.
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR