JP-3790 add residual fringe correction to extract1d table

[jemorrison]

Resolves JP-3790

Closes #8920

This PR adds three columns to the extracted 1d spectra for MIRI MRS data. These three columns are related to residual fringe correction. It is assumed that a new parameter pars reference file will be delivered for MIRI MRS spec3 which will set ifu_rfcorr = True. If the IFU cube that the spectra is being extracted from is for a single band then the residual fringe corrected data is reported as 3 additional columns. If the data is not for a single band then Nan values are written to these three additional columns. For default spec2 extracted spectrum the data does not cover 1 band (but it is for 2 channels) so the residual fringe corrected data will not be written to the x1d files - Nan values will be written.

The spectral leak step was updated to accept MRSMultiSpecModel.

The spectral leak has no unit tests. A separate JP ticket will handle adding various unit tests for this step.
https://jira.stsci.edu/browse/JP-3911

Other steps that use MRS x1d data was reviewed to see if it need to be updated to accept MRSMultSpecModel data.
The master background step and combine 1d step use datamodels.open to open the X1d files. The Flux column is read from the x1d files. No change in these steps is required.

This PR also updated the moving target retests that had an error in them. This why there are regression test failures with this mode

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
• Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
  • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job ([click here for instructions]
    https://github.com/spacetelescope/RegressionTests/actions/runs/13664659037
  • (https://github.com/spacetelescope/RegressionTests/blob/main/docs/running_regression_tests.md))
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

news fragment change types...

• changes/<PR#>.general.rst: infrastructure or miscellaneous change
• changes/<PR#>.docs.rst
• changes/<PR#>.stpipe.rst
• changes/<PR#>.datamodels.rst
• changes/<PR#>.scripts.rst
• changes/<PR#>.fits_generator.rst
• changes/<PR#>.set_telescope_pointing.rst
• changes/<PR#>.pipeline.rst

stage 1

• changes/<PR#>.group_scale.rst
• changes/<PR#>.dq_init.rst
• changes/<PR#>.emicorr.rst
• changes/<PR#>.saturation.rst
• changes/<PR#>.ipc.rst
• changes/<PR#>.firstframe.rst
• changes/<PR#>.lastframe.rst
• changes/<PR#>.reset.rst
• changes/<PR#>.superbias.rst
• changes/<PR#>.refpix.rst
• changes/<PR#>.linearity.rst
• changes/<PR#>.rscd.rst
• changes/<PR#>.persistence.rst
• changes/<PR#>.dark_current.rst
• changes/<PR#>.charge_migration.rst
• changes/<PR#>.jump.rst
• changes/<PR#>.clean_flicker_noise.rst
• changes/<PR#>.ramp_fitting.rst
• changes/<PR#>.gain_scale.rst

stage 2

• changes/<PR#>.assign_wcs.rst
• changes/<PR#>.badpix_selfcal.rst
• changes/<PR#>.msaflagopen.rst
• changes/<PR#>.nsclean.rst
• changes/<PR#>.imprint.rst
• changes/<PR#>.background.rst
• changes/<PR#>.extract_2d.rst
• changes/<PR#>.master_background.rst
• changes/<PR#>.wavecorr.rst
• changes/<PR#>.srctype.rst
• changes/<PR#>.straylight.rst
• changes/<PR#>.wfss_contam.rst
• changes/<PR#>.flatfield.rst
• changes/<PR#>.fringe.rst
• changes/<PR#>.pathloss.rst
• changes/<PR#>.barshadow.rst
• changes/<PR#>.photom.rst
• changes/<PR#>.pixel_replace.rst
• changes/<PR#>.resample_spec.rst
• changes/<PR#>.residual_fringe.rst
• changes/<PR#>.cube_build.rst
• changes/<PR#>.extract_1d.rst
• changes/<PR#>.resample.rst

stage 3

• changes/<PR#>.assign_mtwcs.rst
• changes/<PR#>.mrs_imatch.rst
• changes/<PR#>.tweakreg.rst
• changes/<PR#>.skymatch.rst
• changes/<PR#>.exp_to_source.rst
• changes/<PR#>.outlier_detection.rst
• changes/<PR#>.tso_photometry.rst
• changes/<PR#>.stack_refs.rst
• changes/<PR#>.align_refs.rst
• changes/<PR#>.klip.rst
• changes/<PR#>.spectral_leak.rst
• changes/<PR#>.source_catalog.rst
• changes/<PR#>.combine_1d.rst
• changes/<PR#>.ami.rst

other

• changes/<PR#>.wfs_combine.rst
• changes/<PR#>.white_light.rst
• changes/<PR#>.cube_skymatch.rst
• changes/<PR#>.engdb_tools.rst
• changes/<PR#>.guider_cds.rst

[codecov]

Codecov Report

Attention: Patch coverage is 57.81250% with 27 lines in your changes missing coverage. Please review.

Project coverage is 72.51%. Comparing base (5695d36) to head (21f55f9).
Report is 113 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/extract_1d/ifu.py	72.41%	8 Missing ⚠️
jwst/spectral_leak/spectral_leak.py	0.00%	7 Missing ⚠️
jwst/spectral_leak/spectral_leak_step.py	0.00%	7 Missing ⚠️
jwst/extract_1d/extract_1d_step.py	76.19%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9073      +/-   ##
==========================================
- Coverage   73.67%   72.51%   -1.17%     
==========================================
  Files         369      371       +2     
  Lines       36407    37092     +685     
==========================================
+ Hits        26824    26896      +72     
- Misses       9583    10196     +613     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[drlaw1558]

One concern with changing this to True is that it would then require failsafe cases for all non-MRS uses of extract1d, which may have unintended consequences. May be easier to keep as False, since the only purpose of the flag now is to print a message saying how the flag isn't necessary any longer.

[jemorrison]

I have not added any print statement that says the flag is not necessary. Do we want that still ?. Since we are keeping the flag it seemed like something that might confuse the user. It never hurts to set it to True and we are not going to remove the ifu_rfcorr now. But I can add a flag that says 'By default ifu_rfcorr is true now on need to set it.' If you think it will help

Since the rf_corr is only done in ifu.py (ifu_extract1d) it only operates on MIRI and NIRSPEC. I turned off the ifu_rfcorr for all data but MIR_MRS in extract_1d_step (so NIRSPEC data).
That should do it. I ran it on a few NIRSpec IFU files I had and it works as expected.
But I will run the regression tests to make sure

[drlaw1558]

Given the approach we're now taking I think this change is ok without print statements, but let's add 'for MIRI MRS data' to the comment to make clear it only affects that mode.

[jemorrison]

@drlaw1558 I think this PR is was is needed. I am going to run the regression tests to make sure all the NIRSPEC data is the same.

[drlaw1558]

@jemorrison Looks like a couple issues at the moment.

• rf_flux (and other rf quantities) aren't getting calculated unless the step is passed ifu_rfcorr=True. We want this to run all the time for MRS spectra, ignoring the ifu_rfcorr setting.
• All that the ifu_rfcorr flag should do is to print a message saying that the flag does nothing any longer, and that RF-corrected spectra are automatically present in the x1d data.
  In a later build we'd then remove the ifu_rfcorr step option, we're just going through a temporary grace period to allow folks to adapt to the new usage so that their notebooks don't crash if they try to set a no-longer existing option.

[jemorrison]

@drlaw1558 I am assuming you do not want the rf correction run on NON-single band data. So in spec2 or if a user created multi band cubes. That is why I left to turn on and off. I was thinking the parameter reference file in spec3 would turn it on.

That seems the cleanest. Are you thinking something else?

[drlaw1558]

@jemorrison Gotcha; my original aim was ultimately to be able to retire the flag entirely, so that users never need to see or interact with it. The rfcorr columns would just be something that may or may not be better than the regular columns (depending on how well the algorithm performs in each case). The existing code already can cleanly handle the case of being passed per-channel or all-wavelength cubes, returning unaltered vectors, and we could make sure that it exits cleanly in the spec2 cube case as well though.

I see the point that you're making though; we could keep the ifu_rfcorr argument and just set it to True by default in spec3 param files. One problem with that though is that this wouldn't cover the use case of rerunning just Extract1D on cubes that spec3 already created (which I certainly use in a number of notebooks). We could add ifu_rfcorr=True to the extract1d param file, but that would then try to turn it on for spec2 version of extract1d too (or users running Extract1D on spec2 cubes). That might be ok though since there's a block in the code to set it to FALSE for spec2 cubes...

Thoughts?

[jemorrison]

@drlaw1558 We could just do as you first suggested and run the residual fringe correction on all data and populate the 3 columns of the x1d. We are going to have to update the documentation to describe these new columns. We could put the information in there that residual corrected columns are really only for single band cubes. I do worry people will miss that information.

Or we could set rf_corr to true in extract1d step (default = True) or the parameter file as you suggest and allow the code to determine when it should not be run (i.e. the band is not single). With a clean log message that say turning off residual fringe correction and add that information to the docs.

I say pick the one you think will be the clearest/easiest for the users

[melanieclarke]

Or we could set rf_corr to true in extract1d step (default = True) or the parameter file as you suggest and allow the code to determine when it should not be run (i.e. the band is not single). With a clean log message that say turning off residual fringe correction and add that information to the docs.

This would be my preference if possible: ignore the flag as David first suggested and determine inside the code if it's possible to usefully run the rfcorr algorithm. If so, run it and store the results in the new columns. If not, store NaNs.

[jemorrison]

@melanieclarke @drlaw1558
I think this works:
rf_corr=True by default in extract_1d_step.py.
The extract_1d_step will check if the MRS data is a single band. If it is not then rf_corr is set to false
and prints a log message saying it is turned off is printed.
If ifu_rfcorr is false then the 3 new columns in the spec table are set to NULL.

This will allow the user to run extract_1d on an IFUcube and the program determines if the residual fringe correction can be run and they do not need worry about setting it to false. If they did set it to false then the columns in the spec table will be NULL.

Does this work for all the cases ?

[drlaw1558]

@jemorrison I think that works for me; no need to update param ref files and we keep the existing logic flow. Anyone working with MRS data who doesn't override it will get rf-corrected spectra automatically in the cases where it's possible to create them.

We should ensure though that we've caught all of the cases where rfcorr needs to be disabled within the code. I.e., check that it's turned off for everything except MRS data (if we aren't already). We should also be sure that we catch the failure cases.

I can see one potential problem in the code; the flux, surf_bright, and background calls to residual fringe are all embedded in their own try/except blocks, but we only have a catch below for if the temp_flux_rf calculation failed. If that calculation succeeds but the surf_bright or background calculations fail, then the zip statement down below will fail. In the ton of data that will now be going through this routine, I'm sure that condition will occur from time to time. So, we should have some kind of catch for whenever any of those three fail out.

[jemorrison]

@drlaw1558 Good catch on that possible error of the way the try except was coded.
Now all 3 arrays are checked if one of them is None - then all are set to Nan.

[melanieclarke]

And one more thing - running regtests locally, it looks like there is a sneaky bug in the regression tests in test_miri_mrs_spec3.py. They are reporting no differences, despite creating a totally different datamodel with 3 new columns!

I think I tracked it down to this line:
rtdata.get_truth(os.path.join(TRUTH_PATH, rtdata.output))

It needs to be this:
rtdata.get_truth(os.path.join(TRUTH_PATH, output))

The first line copies the newly created output file as the truth and therefore finds no differences. The second one appropriately retrieves the truth file from artifactory and then finds the expected differences.

[jemorrison]

@melanieclarke
Just what to make sure I understand the change needed for the regtest test_miri_mrs_spec3.py
The line before rtdata.get_truth sets
rtdata.output = output
Every test in test_miri_mrs_spec3.py works like this.
So what I am not understanding ?

[melanieclarke]

@melanieclarke Just what to make sure I understand the change needed for the regtest test_miri_mrs_spec3.py The line before rtdata.get_truth sets rtdata.output = output Every test in test_miri_mrs_spec3.py works like this. So what I am not understanding ?

That's the sneaky part -- when rtdata.output is set, it automatically modifies the input value to turn it into an absolute path. Even though it looks like rtdata.output equals output, it doesn't. All of the tests in test_miri_mrs_spec3.py and test_miri_mrs_spec3_moving_target.py need to be changed to use the relative path in output instead of the absolute path in rtdata.output.

[jemorrison]

@melanieclarke I have a bunch of conflicts now with ifu.py and I made many commits.
How do I avoid having to change every commit and only do it for the last one ?
I think that is either how you or Ken suggested doing it

[melanieclarke]

How do I avoid having to change every commit and only do it for the last one ?

If it's too many or too difficult to rebase, you can just merge instead and resolve the conflicts once.

[jemorrison]

So do you first do a 'git fetch jwst '
then 'git merge '

[jemorrison]

@drlaw1558 I am updating the spectral_leal correction to work on residual fringe corrected data
Do we need to catch that a user might trying to run spec3 on data processed earlier so there does not exist the columns for
residual fringe corrected data ?

[drlaw1558]

@jemorrison Wouldn't hurt to check for it, users can do many unexpected things...

[jemorrison]

@drlaw1558 The masterbackground step reads in the FLUX column from the spec table. If we want that step to use the residual fringe corrected flux we will need to add that.

@melanieclarke I think both master background and combine1d will run fine because datamodels.open is used to open the x1d files. They are not assumed to be a MultiSpecModel. The documentation has assumed they are MultiSpecModel. The code will run (will confirm with regression testing) the FLUX column is used to access to residual fringe corrected data for master background. But that is probably fine - for now.

[drlaw1558]

@jemorrison We want the master_background step to continue reading the FLUX column since the subtraction will happen in cube space.

[jemorrison]

regression tests:
https://github.com/spacetelescope/RegressionTests/actions/runs/13664659037

[jemorrison]

@tapastro @melanieclarke This PR is ready for a final review.
The regression tests located at:https://github.com/spacetelescope/RegressionTests/actions/runs/13664659037
have the expected failures of the 3 additional columns in the x1d data for the MRS. Since the moving target regression test was updated and a bug in testing the files was removed there are moving target failures.

[melanieclarke]

Looking good! Thanks for the new spectral_leak regtest with and without the extra columns - it's good to have that covered.

I just have a couple more small clean up suggestions.

Regression test results look as expected for the new columns and the regtest bug fixes.

[melanieclarke]

Thanks for all the updates! LGTM.