Wrap "process_slit" in try/except for "extract_2d.nirspec.nrs_extract2d" #8964
Conversation
|
Hello, and thanks for sending this in! The try/except change to extract_2d sounds pretty benign, but I'm not sure why there are changes to set_telescope_pointing in this PR as well. Were those included in error? |
|
Oh, I guess that was an earlier commit to my fork where I had implemented providing an explicit MAST_TOKEN to various functions there. Is it possible to just remove the changes to that file from this PR and just keep the changes to extrac2d? I'm not sure I've ever done that before on GH. |
Yes, that's possible! Just make the changes in your own local version and push them up to the same branch - the PR will automatically update. If you make the changes via rebasing or editing old commits, you may need to add a force-push flag when you push up the changes to your fork ( |
|
Thanks for the help. I've removed the other change so now this PR is only for the small update to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8964 +/- ##
==========================================
- Coverage 64.52% 64.52% -0.01%
==========================================
Files 375 375
Lines 38739 38743 +4
==========================================
+ Hits 24997 24998 +1
- Misses 13742 13745 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
This needs a change log entry - please add a one-line summary to a file called 8964.extract_2d.rst, in the changes directory.
One quick suggestion for formatting the warning string, but other than that, I think this looks fine. I'll run regression tests. @hayescr - any objections from NIRSpec?
| 'process_slit failed for slit/subarray {0}, '.format(slit.name) | ||
| + 'probably because the cutout has no valid pixels' |
There was a problem hiding this comment.
This would be a little simpler formatted as an f-string:
| 'process_slit failed for slit/subarray {0}, '.format(slit.name) | |
| + 'probably because the cutout has no valid pixels' | |
| f'process_slit failed for slit {slit.name}, ' | |
| f'probably because the cutout has no valid pixels' |
|
This looks reasonable to me as well, though the only thought I have is whether it's possible to do a more specific catch that we could be doing when building the cutouts (e.g., checking whether the unpadded pixels fall on a detector). I'd be happy to take a look if you could let me know what data and PRISM wavelength range you were running into these errors @gbrammer. |
|
|
@gbrammer - just an update: we're still looking into this, but need to move it to the back burner for a week or two. We'll get back to you when we have availability again. |
|
Okay I took a look and it looks like the issue is that when assign_wcs determines which slits are valid and fall on the detector, it doesn't check to see if the 2D cutouts have valid wavelengths yet. When the assign_wcs step runs assign_wcs.nirspec.compute_bounding_box that it does not yet have an inverse for the slit2detector, so it won't run this block: https://github.com/gbrammer/jwst/blob/0e2337d88f7a11045a0c4e71161152deeebf84ce/jwst/assign_wcs/nirspec.py#L1392C1-L1398C1 For the slit that causes errors, extract_2d will run that block, and finds that the wavelengths are all NaNs because it's at the edge of the buffer region, and will therefore find that there is no valid updated y_range and crash. One possible solution would be to check y_range, and if it is empty, don't update the bounding box, and then instead of adding the try/except in extract_2d, run a check on input_model.wavelength and skip any slits that have no valid wavelengths, if that sounds reasonable? One other issue I ran into was that even with one of these two fixes, I am getting an error in resample_spec. I'm not sure if you ran into a problem there as well @gbrammer, but I'll continue to see if I can't sort out why this is happening. |
|
Thanks for diving into this @hayescr. Your proposed solution to check the y_range output and move on if there are no valid wavelengths sounds reasonable. We don't need to keep the empty bounding box for the offending slit, we just need |
This PR addresses behavior in
extract_2d.nirspec.nrs_extract2d, which can fail if no valid pixels are found in a cutout that are then used to define the slit range of the bounding box.This behavior doesn't seem to occur in normal pipeline processing, but I'm working on tests to change (i.e., extend) the wavelength bounds of spectral cutouts. Nearly all pipeline functions seem to work perfectly fine after simply changing the values in the
wavelengthrangereference file, but occasional problems can occur for PRISM slitlets where the padded extended cutout overlaps a particular detector but where there are no resulting valid unpadded pixels used to compute the slit coordinates of the bounding box injwst.assign_wcs.nirspec.compute_bounding_box.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