JP-1136: Compute scaling for WFSS background subtraction using error-weighted mean

[emolter]

Resolves JP-1136
Resolves JP-3806
Closes #8989

This PR improves the way the reference background is compared with the data for NIRCAM and NIRISS WFSS observations during the background step, and improves the runtime of the background step tests by removing redundant calls from unnecessary parametrization. The bulleted points below are the high-level changes:

• The behavior prior to this PR was to sigma-clip the data (taking the middle 50%), sigma-clip the background, take the mean of each, and then do data - mean_sci / mean_bkg. The new behavior, as of this PR, is:

1. compute

factor = sum(data*model_bkg/var) / sum(model_bkg*model_bkg/var)
sub = data - factor*model_bkg

where var is the sum of all the error terms in the data. This factor is akin to a variance-weighted mean.
2. Check sub against a variance criterion to see if it's a good fit.
3. Reject outliers in the data using a sigma-clipping algorithm, initially set so the middle 98% of data values are kept.
4. Repeat steps 1-3 until either a max number of iterations is reached, or the variance did not improve (c.f. a percent threshold) since last iteration.

• Three new top-level step parameters are introduced to control the iterative outlier rejection process during background subtraction - see documentation for the parameter definitions.

• During testing of this change, it was noticed that the WFSS unit tests were not set up in a way that allowed assessment of the goodness-of-fit of the background subtraction. In short, they were using mock data that just contained noise and comparing it with reference backgrounds from CRDS that contained substantial variations due to real detector imperfections. This PR makes it so that a fake background with well-known variability is generated on-the-fly, then saved to a file for subtract_wfss_bkg to read. A positive side-effect of this change is that it decreases the number of CRDS queries run by the unit tests, so they run faster.

• Unit test parametrizations that caused the background step to be called for every pupil/filter/detector combination were removed, because the code takes an identical path irrespective of the value of these parameters. The only difference was that different background reference files were found in CRDS, but unit testing is not the place to ensure those queries work correctly (and the tests themselves did not meaningfully test this beyond ensuring that the step runs). This substantially improved the runtime of these tests. Calls to each GRISM were retained for each mode because the grism changes the dispersion direction.

• This was definitely beyond the scope of the initial tickets, but it seemed logical to separate the WFSS background subtraction into its own file because it doesn't use any of the same functionality as the background subtraction for the other modes, and the iterative outlier rejection added a fair amount of code. I went ahead and did that refactor but I'm happy to revert it if desired.

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

[emolter]

initial regtests started here: https://github.com/spacetelescope/RegressionTests/actions/runs/12144260342

[codecov]

Codecov Report

Attention: Patch coverage is 92.37288% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.76%. Comparing base (e3d263f) to head (841c516).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/background/background_sub_wfss.py	92.03%	9 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e3d263f) and HEAD (841c516). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (e3d263f)	HEAD (841c516)
	2	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #8990       +/-   ##
===========================================
- Coverage   76.81%   66.76%   -10.05%     
===========================================
  Files         496      376      -120     
  Lines       45610    37994     -7616     
===========================================
- Hits        35034    25366     -9668     
- Misses      10576    12628     +2052     

Flag	Coverage Δ
nightly	?

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

[emolter]

regtests here https://github.com/spacetelescope/RegressionTests/actions/runs/12283134985

[Rplesha]

Adding in Gaël's review while we wait for his GitHub account to be added to the spacetelescope group.

The code itself appears correct as written, although he has not had time yet to run a test of the code.

[melanieclarke]

I have not tracked the conversation for this one, so I'm reviewing mostly for surface-level details. The algorithm looks plausible, and regtest changes for NIRCam and NIRISS WFSS look reasonable.

Separating out the WFSS background subtraction into a separate file looks like a good idea to me. I have a few small suggestions below.

Also, test coverage looks pretty good for the changes, but it would be nice to add a test or two for WFSS at the step level.

[emolter]

it would be nice to add a test or two for WFSS at the step level.

Can do, and will work on that tomorrow morning.

[emolter]

@melanieclarke I added some top-level tests as you suggested. I also went ahead and finished the changes relevant to JP-3806, i.e., to remove the looping over the various filter/pupil/detector combinations - those do not cause the code to do anything different besides retrieve different background reference files, so I don't think unit tests are the place for that. Let me know what you think.

I'll run another set of regression tests after incorporating any more comments.

[melanieclarke]

Thanks for the new tests! I'm fine with not iterating over the filters for the WFSS unit test - I agree that doesn't add much.

I'd still like to consider changing the step parameter names, and I'd like to hear from the science team that the algorithm as implemented meets their needs, but I think this otherwise looks fine from a software perspective.

[emolter]

I'd still like to consider changing the step parameter names

Me too, I'll just go ahead and do it. Will plan to go with whatever comes to mind unless you have suggestions

[Rplesha]

I believe @gnoir0t is testing the change on the NIRISS side, but let me know otherwise Gaël, and I can calibrate some NIRISS data to look at today.

@Russell-Ryan and/or @NorPirzkal are you working on calibrating data for NIRCam to test the output scientifically? Do you need any help doing so?

[Russell-Ryan]

We have not tested the pipeline version of things. But the algorithm of the inverse variance weighting is something we've done many times. At present, the NIRCam background images need a bit of work and I'm nearly finished with the four most common broadband+grism combinations. I can test the pipeline next week and will have fresh files to deliver soon.

[tapastro]

Regression tests here: https://github.com/spacetelescope/RegressionTests/actions/runs/12358452874

Expected failures across wfss tests due to change in background subtraction, as well as some failures likely due to connection issues.

[tapastro]

The default described here does not match the default value in the step spec - from the discussion on the ticket, it appears as though this wasn't concretely specified. I think it would be reasonable to set to this to 1, but if you think 0 is more intuitive, I would not object. We just need the two locations to match.

[emolter]

This is an RMS stopping criterion, so only stopping when delta RMS is zero seems more intuitive to me. 1 could be a valid value, and it seems that INS wants to run up to the maxiter criterion by default. But I will check where the default documented here does not equal the spec

[gnoir0t]

Yes I also noticed this discrepancy, where in the code the default is set to 1. For the NIRISS tests I've performed, a default of 1 or 0 makes no difference as in both cases the code reaches the default maxiter of 5. But to be safe it could be 0 rather than 1.

[gnoir0t]

I made various tests of the changes on some NIRISS science data, testing various values and combinations of values for the three new parameters (the max iter number, the outlier rejection percentage, and the rms threshold) and the code seems to work as intended. For the NIRISS data I tested, the default values seem to perform slightly better than the previous scaling method, and tweaking those (eg, higher maxiter, or outlier rejection percentage, or small rms threshold with high maxiter) allow the user to improve some more the residuals of the background-subtracted images.