JP-3598: The ResampleSpecStep and Extract1DStep within MasterBackgroundMosStep are inaccessible

[penaguerrero]

Resolves JP-3598
and
Resolves JP-3748

Closes #

This PR adds resample_spec and extract_1d as steps of the MOS background pipeline so that their respective step attributes can be modified there as well.

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)
    • 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 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.54%. Comparing base (b245652) to head (099eb55).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...st/master_background/master_background_mos_step.py	87.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8847      +/-   ##
==========================================
- Coverage   64.54%   64.54%   -0.01%     
==========================================
  Files         375      375              
  Lines       38740    38739       -1     
==========================================
- Hits        25004    25003       -1     
  Misses      13736    13736              

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

[penaguerrero]

Reg tests running: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1766/

[penaguerrero]

The error in the regression tests is as expected and can be ignored: Extra keyword 'S_PXREPL' in a: 'SKIPPED'

[melanieclarke]

Code changes look good to me. I'm attempting to test now with some real data.

Can you please update the documentation for master background with MOS to add in the pixel_replace step? Also, it looks like there is not yet any mention of how to set parameters in the substeps for the master_background_mos pipeline -- can we add a sentence or two about that to the description?

[melanieclarke]

On closer look, and testing on real data, I think this needs more work. Setting the other steps up as part of the step_defs allows it to take input parameters for the substeps, but then they are not actually used, since the steps are created and called in create_background_from_multislit instead of in the process function, like the other steps are.

I'm testing by just running spec2 from the command line on a MOS rate file with background slits, setting --steps.master_background_mos.steps.pixel_replace.skip=False. The top-level parameters report that skip is set to False, but when the step is created later, the default parameter skip=True is used.

[melanieclarke]

One more thing - this needs documentation updates, in docs/jwst/master_background/description.rst, under the NIRSpec MOS Mode heading.

[hayescr]

@melanieclarke - either way is fine with me.

I'll take a look at this PR to see if I have any feedback, but one thing I did notice was the switch to call the combine_1d step directly, and I want to get your all's thoughts on this. The only parameter available for this step currently is the exptime_key, and all of the options should perform identically for the MOS background spectra (because all of the spectra have the same integration time and exposure time so the weighting will be the same for all spectra, similar to the unit weights option), so it's not really necessary to provide access to this option.

#8932 will add another parameter for combine_1d.combine_1d_spectra that we will want to have different defaults when called by master_background_mos vs. the combine_1d step. Doing so is straightforward if both steps call combine_1d_spectra with their own default settings, but it might be less straightforward if calling the combine_1d step in master_background_mos. So I was wondering what you all thought of keeping the combine_1d_spectra function call instead of calling the step?

[melanieclarke]

So I was wondering what you all thought of keeping the combine_1d_spectra function call instead of calling the step?

This was my suggestion/fault - I figured it would be more forward looking if the combine_1d call was part of the pipeline too. That way if we add more parameters to it in the future, the user can directly control them as needed.

For the case where you want master_background_mos to have a different default for specific combine_1d parameters, we could pass them directly from the top-level parameters to the step before calling it, e.g.

self.combine_1d.sigma_clip = self.sigma_clip
master_background = self.combine_1d.run(bkg_model)

I can see that potentially being confusing, though, if the user sets sigma_clip directly in the substep, and then it's overridden by the top-level parameters.

Since combine1_d is currently a simple step, and there is already the self-contained combine_1d_spectra function, it would be fine by me to leave it as is.

[penaguerrero]

@melanieclarke and @hayescr I merged the changes of #8932. Can you please take a look at the code?

[melanieclarke]

There are some unit tests that are failing because they call the create_background_from_multislit on a slit model, rather than extracted spectra, as is now expected. Can you please update those?

[hayescr]

I don't think I have any other comments on top of what Melanie mentioned and the changes work for me. In terms of the tests, just in case it helps streamline adjusting the tests, I had set up a few of them to work around applying/removing some of the NIRSpec correction steps. I just did this to avoid the effects of the pixel-to-pixel differences in the flat-fielding, which can make the values of the background and background correction difficult to predict ahead of time.

[melanieclarke]

Testing locally, it looks like parameters for the substeps are now being passed appropriately and the new parameters for sigma clipping and median filtering are incorporated correctly.

There are still some lingering clean up issues - please address these, and then we should re-run regression tests.

[penaguerrero]

Regtest: https://github.com/spacetelescope/RegressionTests/actions/runs/12147624590

[melanieclarke]

All comments addressed and regtest results look good - the only change is that the pixel replace keyword is added in the background spectra (and set to SKIPPED, as expected).