JP-3673: Update background-subtraction intermediate products for WFSS #9772
Conversation
penaguerrero
added
no-changelog-entry-needed
8.1 patch
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9772 +/- ##
==========================================
+ Coverage 82.49% 82.51% +0.02%
==========================================
Files 366 366
Lines 37134 37140 +6
==========================================
+ Hits 30633 30647 +14
+ Misses 6501 6493 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Allowing the cal_step status to be FAILED has some implications for the record_step_status function in jwst/stpipe/utilities. It currently assumes there are only two options: COMPLETE and SKIPPED. Since we are only allowing this value for this one step right now, which does not use record_step_status, I think we can address this later, but we should file a ticket for it.
It also looks like this step could use some more clean up on when it makes a copy of the input_model. In addition to the places I noted below, it also looks like subtract_soss_bkg is being called on input_model, when it should be called on the copy. The last three tests in test_background_soss.py should be modified to include this line to verify that the output is not a shallow copy of the input:
assert mock_model.meta.cal_step.bkg_subtract is None
If I add that line locally, those tests would currently be failing.
|
Thanks for the clean up! Can you please also modify the tests to add the check for a shallow copy? |
|
Here's the follow up ticket to more broadly address possible FAILED status: |
| @@ -179,6 +179,7 @@ def test_nirspec_gwa(tmp_cwd, background, science_image): | |||
| assert_allclose(result.data, test) | |||
| assert type(result) is type(science_image) | |||
| assert result.meta.cal_step.bkg_subtract == "COMPLETE" | |||
| assert result is not science_image | |||
There was a problem hiding this comment.
This test is a good start, but it still won't catch a shallow copy, because the top-level object is not the same, but the underlying data is still shared. In all these cases, we should also add something like this to check that the meta was not updated in the input:
assert science_image.meta.cal_step.bkg_subtract is None
|
|
||
| # Test image-based correction. | ||
| result = subtract_soss_bkg(generate_soss_image, template_model, 35.0, [25.0, 50.0]) | ||
| assert result is not image |
There was a problem hiding this comment.
This test isn't helpful, since image is a copy already. You need to check that result is not generate_soss_image.
| result.meta.cal_step.bkg_subtract = "SKIPPED" | ||
| self.log.warning("Skipping background subtraction") | ||
| self.log.warning( | ||
| "GWA_XTIL and GWA_YTIL source values are not the same as bkg values" | ||
| ) | ||
|
|
||
| input_model.close() | ||
| model.close() |
There was a problem hiding this comment.
I don't think we want to close model here - it should be the same model as result, which we want to return open.
Resolves JP-3673
and
Resolves JP-4062
This PR addresses saving the background mask in the corresponding datamodel extension, even if the step exists early. In such case, the scaling factor keyword will be saved
with value "N/A", and the status of the step will be set to SKIPPEDwith value 1.0 and the status of the step will be set to FAILED.Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)docs/pageokify_regteststo update the truth files