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-3796: Scale barshadow correction to long slits #9085

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

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.

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 (ed4bac8) to head (b8e1511).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9085      +/-   ##
==========================================
+ Coverage   73.73%   73.76%   +0.02%     
==========================================
  Files         373      373              
  Lines       37275    37249      -26     
==========================================
- Hits        27485    27475      -10     
+ Misses       9790     9774      -16     

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

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).

jwst/assign_wcs/nirspec.py Show resolved Hide resolved
jwst/assign_wcs/nirspec.py Outdated Show resolved Hide resolved
jwst/barshadow/bar_shadow.py Show resolved Hide resolved
jwst/barshadow/bar_shadow.py Show resolved Hide resolved
@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
"closed_open": shadow1x1[:501, :],
"closed_closed": np.nanmin(shadow1x1) * np.ones((501, 101)),
"last": shadow1x1[501:, :],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was never very happy with hardcoding numbers like 500, 501 and 101 but I never got around to relating them to the dimensions of the arrays in the reference file or the provided WCS information. Is it worth trying to do that here, or should we just open a new ticket for a future improvement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should open a new ticket for that. For now, the NIRSpec team is aware that the barshadow file needs to have a fixed dimension, and we should discuss with them if we want to make other assumptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I filed a ticket here: https://jira.stsci.edu/browse/JP-3868

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.

5 participants