-
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-3102: NIRSpec combined MOS/FS processing #8467
JP-3102: NIRSpec combined MOS/FS processing #8467
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8467 +/- ##
==========================================
+ Coverage 58.63% 59.50% +0.87%
==========================================
Files 391 391
Lines 39081 39245 +164
==========================================
+ Hits 22914 23354 +440
+ Misses 16167 15891 -276 ☔ View full report in Codecov by Sentry. |
6d6fc0a
to
0e75426
Compare
31897e1
to
8dce641
Compare
ddaf8d7
to
d06baa7
Compare
e98b3a7
to
8429797
Compare
56dd6b5
to
cc14cfe
Compare
…ithout MSA file source definition
Okay, I've added documentation everywhere I could think of, and I have merged in the wavecorr changes, so I'm going to take this out of draft now. New regression test run started here: Testing locally, I see the changes I expected for test_nirspec_mos_spec2, due to rearranging the slit order. After my last update, I now see no changes for test_nirspec_fs_spec2, except some pixel_replace differences that also appear when testing with master. I see no changes for test_nirspec_fs_spec3 or test_nirspec_mos_spec3. |
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.
Comments on review so far. Still much more wade through, but don't want to lose these.
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.
Wow, what a lot of great work! Especially all the new (extensive) unit tests.
Looks like you could use one more regtest run, now that truth files have been updated for unrelated changes. |
Good idea - I'll start another one when the system is free. Started here: |
@melanieclarke I'm looking over this in the next couple days and will let you know whether I have any comments. |
Latest regression test results show expected NIRSpec MOS spec2 changes for reordering the slits, plus a change I previously overlooked for NIRSpec FS master_background correction. The change is because the input test data is a cal file containing multiple fixed slits all marked POINT. Before this PR, point sources in non-primary FS were partially handled in the master_background step: it expected to find and use a pathloss extension for both point and uniform sources to correct the background, but did not expect or use flat or photom corrections. This partial correction should never be encountered in the wild anymore: non-primary sources are always marked EXTENDED now. Since the source locations can't be determined, none of the point source corrections are relevant, including pathloss. In this PR, I changed the master_background handling to expect that a point source has all the necessary extensions (flat, pathloss, and photom), or else the background is not corrected for a point source. It will warn, and use the background directly, as if the source were extended. This is the source of the slight change in the regtest: the existing pathloss point corrections in the test data are ignored when computing the background for the non-primary slits. |
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.
Thanks for the thorough work @melanieclarke! The changes look good and in testing this with some data everything works/looks as I would expect it to.
Thanks for testing @hayescr! |
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 great to me!
Co-authored-by: Howard Bushouse <[email protected]>
JP-3564: remove most deep copies of NIRSpec WCS objects implements option for polyfit smoothing, save here, remove next commit JP-3581: initial commit of minimal working example use pipeline to handle backgrounds in association instead of step fixed unit tests, improved output filename handling fixed pipeline failures when step skipped added docs attempted fix readthedocs build responding to @melanieclarke and @drlaw1558 comments added support for selfcal files in asn responding to @drlaw1588 additional comments requested variable name change in badpix_selfcal.py responding to @melanieclarke and @braingram comments bugfix to unit test responding to next round of @melanieclarke comments added spaces to log warnings one more round of comments, thanks @melanieclarke fixes in response to @braingram comments updated regtest to include intermediate files split regtest in two responding to @hbushouse comments pin numpy add changelog Update stcal requirement from <1.6.0 to <1.8.0 (spacetelescope#8571) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> JP-3102: NIRSpec combined MOS/FS processing (spacetelescope#8467) Co-authored-by: Howard Bushouse <[email protected]> JP-3659: Fix for output WCS when first data set is empty (spacetelescope#8562) Co-authored-by: Howard Bushouse <[email protected]> remove requirements-max.txt remove end in ami log messages (spacetelescope#8568) Do not set conda version in Jenkinsfile(s) (spacetelescope#8585) JP-3618: Add intermediate LRS slit wcs reference frame (spacetelescope#8475) Co-authored-by: Howard Bushouse <[email protected]> unpin stsci.imagestats (spacetelescope#8577) Co-authored-by: Howard Bushouse <[email protected]> JP-3660: extract1d _coalesce_bounds fix (spacetelescope#8586) Co-authored-by: Timothy Brandt <[email protected]> Co-authored-by: Howard Bushouse <[email protected]> Fixed a missing import statement Fixed incorrect statement order Removed orphan import statement
Resolves JP-3102
Closes #7769
Process fixed slits defined in MSA metafiles, alongside standard MOS slits.
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR