JP-1478: extract_1d enhancements for NRC_TSGRISM #5414
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5414 +/- ##
==========================================
- Coverage 52.39% 52.38% -0.02%
==========================================
Files 414 414
Lines 37679 37707 +28
Branches 5828 5838 +10
==========================================
+ Hits 19742 19752 +10
- Misses 16676 16695 +19
+ Partials 1261 1260 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
By the way, these changes include a fix for another previously undiscovered bug at https://github.com/spacetelescope/jwst/pull/5414/files#diff-8e03eb4c28c8d698c106149c2306a1becf8f1a7c4e4027bf5dfeae9bc76395abR1782. The wrong object was being returned from the function that was setting up background region coeffs, such that even when the extract1d ref file specified background regions, they were not getting used and hence no background extraction or subtraction was ever actually happening. This leads to differences (good ones!) in at least the NIRISS SOSS regression tests, because it has background regions defined in its extract1d ref file. |
|
OK, the last commit contained a few minor updates to make the new code work for all existing modes and also updated the TSGRISM regression test to use the prototype tsgrism extract1d ref file. That can be reverted once there's a ref file available in CRDS. This is now ready for review. |
|
Of course the changes to step docs and code related to anything for IFU data will get superseded in the near future when 8000 @jemorrison finishes all the updates for IFU extraction. |
There was a problem hiding this comment.
This looks good.
Some specific comments below about test coverage. The extract.py module is only unit tested at the 9% level, which is pretty crazy given it is 3791 lines long. Would be nice to toss in a unit test on the areas you touched so we can start getting some coverage.
| extract_params['bkg_coeff'] = None # no background sub. | ||
| extract_params['smoothing_length'] = 0 # because no background sub. | ||
| extract_params['bkg_fit'] = None # because no background sub. | ||
| extract_params['bkg_order'] = 0 # because no background sub. | ||
| extract_params['subtract_background'] = False |
There was a problem hiding this comment.
It looks like this block is not unit tested. Perhaps a small unit test that just checks that this function works as expected when called with ref_dict=None
There was a problem hiding this comment.
Agreed. Fortunately it is at least tested by 1 or 2 regression tests, specifically the WFSS mode, which does not use an extract1d ref file and hence the ref_dict is None.
| if extract_params['bkg_coeff'] is not None: | ||
| extract_params['subtract_background'] = True | ||
| extract_params['bkg_fit'] = aper.get('bkg_fit', 'poly') | ||
| else: | ||
| extract_params['bkg_fit'] = None | ||
| extract_params['subtract_background'] = False |
There was a problem hiding this comment.
Same here. Not unit tested, but perhaps a small test could target some of this.
There was a problem hiding this comment.
Again, this is at least tested in the regression tests, where the NIRISS SOSS extract1d ref file specifies bkg_coeff values, but none of the other spectral modes do, so both branches do get tested in regtest. But unit test coverage certainly couldn't hurt.
Updates to the extract_1d step to allow for an EXTRACT1D reference file to be used for NRC_TSGRISM mode, so that necessary column-by-column background extraction and subtraction can be performed. Previously the code was hardwired to not use EXTRACT1D ref file params for NRC_TSGRISM mode, which defaulted to very simplistic extraction with no background subtraction.
In addition to enabling the use of the reference file for this mode, a new parameter
bkg_fithas been added, which now allows the user to specify whether to perform the default polynomial fit to background values or do simple mean and median computations. Functions to perform the mean and median background computations have been added to the code. Note that even when mean/median are used, the background model is still created in the form of a polynomial, but in this case it's a zero-order polynomial with the 0th-order coefficient set to the mean/median. That allows the background model to be used as-is in the downstream extraction functions.Documentation for the extract_1d step and EXTRACT1D reference file have also been updated to account for the changes and to give more details on how the various extraction parameters are actually used in the code.
Fixes #5000 / JP-1478