Skip to content

Conversation

@melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Jan 22, 2025

Resolves JP-3796
Partially resolves JP-1341

There are two approximations being made in the barshadow correction that are not scaling well to the very long "slitlet" in MOS data taken in longslit mode.

The first is related to JP-1341: there are two different definitions of slit height. The barshadow correction needs the full bar-to-bar height, but the slit coordinates in the WCS use the open area height. The barshadow code currently hard-codes the ratio between the full and open heights as 1.15, but the value actually varies a bit by quadrant. I'm proposing that we use the equation in JP-1341 to calculate and store the scale factors in the slit datamodel and use the slit.slit_yscale attribute in the barshadow correction in place of the hard-coded value. This requires an stdatamodels PR: spacetelescope/stdatamodels#379. With this change, the bars are much closer to where they need to be for long slit data.

The second approximation is that the barshadow code shifts the slit y-coordinates by the mean value for the bounding box in order to put the center of the constructed shadow at the center of the slit. This is correct if the center of the bounding box is also the center of the middle shutter, but may be a little offset if not. For the long slit test case, it introduced a shift of ~1 pixel. Changing the code to shift the constructed shadow to the fiducial shutter instead of the center of the bounding box fixes this offset as well.

This PR also includes some maintenance and minor refactoring for the barshadow step, to clean up documentation and add unit tests. I also replaced a local implementation of a bilinear interpolation algorithm with scipy.ndimage.map_coordinates: this was a simple, drop-in replacement that performs identically except that map_coordinates preserves NaNs at the edges of the arrays where there are no valid coordinates. The old interpolator extended the barshadow correction all the way to the edge of the data array in the dispersion direction.

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
Copy link
Collaborator Author

melanieclarke commented Jan 22, 2025

Running initial regression tests here:
https://github.com/spacetelescope/RegressionTests/actions/runs/12918138451

Results show differences only for NIRSpec MOS data.
For spec2 tests starting from rate files:

  • The three new FITS keywords are added to all products downstream of extract_2d.
  • Barshadow products and downstream show small numerical differences from the changes in the algorithm.
  • The barshadow extension itself has more NaNs at the edges of the slit due to the change in interpolation edge conditions.
    For spec3 and other miscellaneous tests starting from cal or intermediate products:
  • Only the MSAQUAD keyword appears as a new difference, since the quadrant is available in the datamodel but did not have a FITS keyword before. Slit scales are not available in the existing intermediate products so do not appear.

@codecov
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.76%. Comparing base (c75274b) to head (d31be50).
Report is 865 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9085      +/-   ##
==========================================
- Coverage   77.59%   73.76%   -3.84%     
==========================================
  Files         509      372     -137     
  Lines       46807    37248    -9559     
==========================================
- Hits        36320    27476    -8844     
+ Misses      10487     9772     -715     
Flag Coverage Δ
nightly ?

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@hayescr hayescr left a comment

Choose a reason for hiding this comment

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

Just a few minor comments but otherwise this looks fine to me and the changes to the application of the barshadow correction all look good (and seem to perform well in the cases I tested).

@melanieclarke melanieclarke marked this pull request as ready for review January 31, 2025 21:50
@melanieclarke melanieclarke requested review from a team as code owners January 31, 2025 21:50
@melanieclarke melanieclarke requested a review from nden February 3, 2025 15:32
@melanieclarke melanieclarke force-pushed the jp-3796 branch 2 times, most recently from ffaa392 to 5dba8c8 Compare February 4, 2025 18:55
Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

👏

@melanieclarke
Copy link
Collaborator Author

@melanieclarke melanieclarke merged commit 7a42890 into spacetelescope:main Feb 5, 2025
30 checks passed
@melanieclarke melanieclarke deleted the jp-3796 branch February 5, 2025 17:37
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.

6 participants