JP-3426: Ensure proper order of bitwise operations #8916
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8916 +/- ##
==========================================
- Coverage 64.51% 63.67% -0.84%
==========================================
Files 376 376
Lines 38733 38694 -39
==========================================
- Hits 24990 24640 -350
- Misses 13743 14054 +311 ☔ View full report in Codecov by Sentry. |
| rtdata = rtdata_module | ||
|
|
||
| # Get the input data; load individual data files first, load ASN file last | ||
| rtdata.get_data("nircam/wfss/jw01076-o101_t002_nircam_clear-f356w_spoof_cat.ecsv") |
There was a problem hiding this comment.
What's the difference between this catalog and the original catalog in the test above?
There was a problem hiding this comment.
This is mentioned in the submission text above - it's truncated and has one sky region expanded to cover the full frame.
There was a problem hiding this comment.
Thanks, sorry I didn't read that properly
| @@ -32,6 +32,26 @@ def run_pipeline(rtdata_module): | |||
| return rtdata | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="module") | |||
| def run_pipeline_skip_bkg(rtdata_module): | |||
There was a problem hiding this comment.
The name of this test is confusing because it explicitly sets bkg_subtract.skip to False, maybe specify somehow with the name that you're trying to trigger a skip condition
There was a problem hiding this comment.
I can alter the test name.
| # Run the calwebb_spec2 pipeline; save results from intermediate steps | ||
| args = ["calwebb_spec2", rtdata.input, | ||
| "--output_file='skip_bkgsub'", | ||
| "--steps.bkg_subtract.skip=False"] |
There was a problem hiding this comment.
Is it necessary to test that the cal and x1d truth files are identical? Could you just skip all steps downstream of bkg_subtract and test that the result had bkg_subtract set to skipped?
Why is it necessary to make this test a regression test, instead of a unit test of subtract_wfss_bkg?
There was a problem hiding this comment.
Mostly because the additional computation required to test the pipeline end-to-end in this skipped case doesn't seem sufficiently obtrusive to warrant truncation of the pipeline.
I considered a unit test, but mocking both the rate file and its accompanying catalog file seemed like a headache. Perhaps I was too lazy in making it a regression test.
There was a problem hiding this comment.
I don't think the choice you made is lazy. It just feels like the bugfix in this PR is very specific, affecting one line of code that checks if a skip is need, and the test coverage is very non-specific, which may make it harder to isolate this bug or a similar one if it recurred.
The test for this skip condition could be very simple: you could write a helper function that takes in an input model, mask, and min_n_pixels parameter, then unit-test it by passing in a custom-made mask.
tapastro
force-pushed
the
jp-3426-parens
branch
from
a27e7fc to
da58617
Compare
|
After moving the change to a function and migrating the test coverage to a unit test, new RT run here: https://github.com/spacetelescope/RegressionTests/actions/runs/11578440989 |
Resolves JP-3426
Closes #8061
This PR addresses a reoccurrence of the issue initially seen in the above linked ticket, where the only pixels available for use in calculation of the background are reference pixels, which have the DO_NOT_USE flag set. The initial fix neglected to include some necessary parentheses to ensure that the bitwise operations are done in the correct order. This also exposed the lack of test coverage for this path in the code.
I found the difficulty in preparing a WFSS mock dataset, including a spec model and a source catalog for use in constructing the mask, to be high enough that a regression test seemed the simpler path forward. The new files copy the old files, except to truncate the source_catalog for faster processing and to enlarge the sky region of the first source to encompass the entire data array.
Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst