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 15.62500% with 27 lines in your changes missing coverage. Please review.

Project coverage is 63.69%. Comparing base (0659b9c) to head (7aa82bb).
Report is 8 commits behind head on main.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8847   +/-   ##
=======================================
  Coverage   63.68%   63.69%           
=======================================
  Files         375      375           
  Lines       38691    38691           
=======================================
+ Hits        24640    24643    +3     
+ Misses      14051    14048    -3     

☔ 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.

[penaguerrero]

Ok, step parameters are being updated now inside of create_background_from_multislit

[melanieclarke]

Ok, step parameters are being updated now inside of create_background_from_multislit

That looks like it will almost work for passing the parameters from the top level pipeline to the point where the steps are called, but you're using the call class function to run the step, which creates a new instance of the step instead of running with current parameters. This version is now making the step 3 times: once when it's established as part of the pipeline, once when it's directly instantiated to update the parameters, and once when call is called.

I think we can make the step just once, and avoid having to pass parameters around, if we move the step processing out of the create_background_from_multislit function and call it directly in the process function for MasterBackgroundMosStep. It needs to be called the same way the barshadow or photom steps are called.

[penaguerrero]

@melanieclarke this is the other alternative I had considered. I think it is cleaner.

[melanieclarke]

@melanieclarke this is the other alternative I had considered. I think it is cleaner.

Thank you! I'll take a look.

[melanieclarke]

this is the other alternative I had considered. I think it is cleaner.

Thanks for the updates - I agree, this looks better now!

Two more suggestions:

1. I think we should combine the _set_steps_params with the already existing set_pars_from_parent function. They are doing almost the same thing.
2. We might as well include combine_1d as part of the pipeline too, instead of handling it separately. The user might want control over those parameters too.

[penaguerrero]

1. I think we should combine the _set_steps_params with the already existing set_pars_from_parent function. They are doing almost the same thing.

I think it would be better to leave them in different functions since they do slightly different things, one sets the parameters the same as the parent step, whereas the other just uses the defaults regardless of the parent params

2. We might as well include combine_1d as part of the pipeline too, instead of handling it separately. The user might want control over those parameters too.

I agree, I included it as part of this PR.

[melanieclarke]

I think it would be better to leave them in different functions since they do slightly different things, one sets the parameters the same as the parent step, whereas the other just uses the defaults regardless of the parent params

Is there a reason not to use the parent params, the same way the other steps do? It seems like that would still be the desired starting point for the additional steps.