Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-2928, JP-3792: MIRI LRS master background #8927

Merged
merged 9 commits into from
Nov 4, 2024

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Oct 28, 2024

Partially addresses JP-2928
Resolves JP-3792

Update MIRI LRS regression test data to in-flight examples.

While working with these tests, I noticed a few small bugs relating to master background correction:

  1. If a single spectrum is passed to combine1d, the wavelength array for the input spectrum is sorted in place instead of being copied for the output spectrum. For MIRI, this has the effect of reversing the order of the wavelength array compared to the order of the data, which corrupts the spectrum in the output "combined" product. I added a .copy() to the place where the input wavelength array is read. The combined output spectrum is then the same as the input single spectrum.

  2. The 'mbsub' suffix is used in calwebb_spec3 for the master background output, but it is not recorded as a known suffix, so it does not get properly removed for the next product in the pipeline. I added 'mbsub' to the list of known suffixes.

  3. In the master_background step, when a user background is provided, there were a couple small bugs relating to changes to the ModelContainer in JP-3721: Simplify ModelContainer #8831. There was an overlooked container.update call that crashed if the input was also a ModelContainer, since ModelContainers no longer have their own metadata (so no update function exists). There was also a place where a single model was passed to a function that expected a container when save_background=True, also causing a crash. I fixed these small issues.

For the tests themselves:

For test_miri_lrs_dedicated_mbkg and test_miri_lrs_masterbg_user, I used data from PID 1529 obs 3 and 4, a LRS fixed slit observation with a dedicated background. The background spectrum is from a standard spec3 reduction of obs 4. I added intermediate file saving (save_background = True) for both tests to exercise the above fixes.

For test_miri_lrs_nod_masterbg, I used the standard spec3 ASN file for PID 1530 obs 5, already in artifactory, which specifies background spectra from the alternate nods for the observations, but ignores them for a standard spec3 reduction. To get a non-trivial background subtraction in master_background with these members, it's necessary to run spec2 in a non-standard way:

  • reduce one file at a time, instead of from the spec2 asn, to avoid direct nod subtraction
  • specify a background region in an override reference file for extract_1d, so that the BACKGROUND table is non-zero in the extracted spectrum

For the latter, I created nod1 and nod2 extraction files that swap source and background locations by nod.

Then, it's also necessary to explicitly enable the master_background step in spec3: MIRI param ref files turn it off by default. Instead of just running this whole process offline and storing the inputs to spec3, to mimic the way the current test works, I added a full spec2 run to the master background test with the non-default extraction files. I think this should help make the test for the intended functionality clearer, as well as exercising the background extraction option for the spec2 pipeline -- I'm not sure if that's covered anywhere else in the regression tests.

With these changes, the output products from the test_miri_lrs_nod_masterbg test are comparable to the products from the standard processing of the same inputs in test_miri_lrs_slit_spec3.

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

@melanieclarke melanieclarke added this to the Build 11.2 milestone Oct 28, 2024
@melanieclarke melanieclarke changed the title JP-2928: MIRI LRS master background JP-2928, JP-3792: MIRI LRS master background Oct 28, 2024
@melanieclarke
Copy link
Collaborator Author

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.67%. Comparing base (1cb213a) to head (952846a).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8927      +/-   ##
==========================================
+ Coverage   61.92%   63.67%   +1.74%     
==========================================
  Files         376      376              
  Lines       38691    38691              
==========================================
+ Hits        23961    24638     +677     
+ Misses      14730    14053     -677     

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

@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Oct 29, 2024

Regression tests for test_miri_lrs_nod_masterbg show small differences between the truth products I made on my Mac and the test products on Linux. These are very similar to differences between architectures for the reduction of the same products in test_miri_lrs_slit_spec2 and test_miri_lrs_slit_spec3, so I will okify the Linux results and run again.

New run is here:
https://github.com/spacetelescope/RegressionTests/actions/runs/11575966848

No failures.

@melanieclarke melanieclarke force-pushed the jp-2928-miri-lrs branch 2 times, most recently from f977be7 to 50867f2 Compare October 29, 2024 16:22
@melanieclarke melanieclarke marked this pull request as ready for review October 29, 2024 16:22
@melanieclarke melanieclarke requested a review from a team as a code owner October 29, 2024 16:22
Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question/comment. I like the new regression test

@melanieclarke melanieclarke merged commit 504e16a into spacetelescope:main Nov 4, 2024
26 of 27 checks passed
@melanieclarke melanieclarke deleted the jp-2928-miri-lrs branch November 4, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants