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-3343: Add bounding_box to imaging WCS objects without one #7809

Merged
merged 5 commits into from
Aug 19, 2023

Conversation

tapastro
Copy link
Contributor

@tapastro tapastro commented Aug 11, 2023

Resolves JP-3343

Closes #7820

This PR addresses a help desk inquiry regarding missing FITS SIP approximations for a NIRSpec TAcq image. These values were not filled during assign_wcs because the bounding_box was never defined. This PR is an experiment to see if saving a bounding box to imaging WCS objects of size {data.shape}, when one does not already exist, will break things.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@tapastro tapastro requested a review from a team as a code owner August 11, 2023 19:41
@tapastro
Copy link
Contributor Author

tapastro commented Aug 11, 2023

@tapastro tapastro marked this pull request as draft August 11, 2023 19:44
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (589917d) 76.52% compared to head (4f9a351) 76.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7809      +/-   ##
==========================================
- Coverage   76.52%   76.51%   -0.01%     
==========================================
  Files         456      456              
  Lines       36964    36965       +1     
==========================================
  Hits        28285    28285              
- Misses       8679     8680       +1     
Flag Coverage Δ *Carryforward flag
nightly 77.39% <ø> (ø) Carriedforward from 589917d

*This pull request uses carry forward flags. Click here to find out more.

Files Changed Coverage Δ
jwst/assign_wcs/util.py 83.70% <0.00%> (-0.16%) ⬇️

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

@hbushouse
Copy link
Collaborator

Since this is in response to a Help Desk query, it would be good to retroactively create a JP ticket and link it here. Also helps with the bean counting of how many tickets/bugs we fix! ;-)

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Seems simple and straightforward enough to me. But really need the blessing of the WCS czar (@nden ).

@hbushouse hbushouse requested a review from nden August 15, 2023 13:03
@hbushouse
Copy link
Collaborator

Regtest results show mainly the differences due to a recent update to the refpix step for NRS IRS2 data. One test, for an NRS2 image, does show that the FITS SIP keywords are now included in the cal product header and fully populated. So that's a good sign.

@tapastro tapastro force-pushed the add-nrs-imaging-bbox branch from 7c53032 to 08fd9b8 Compare August 15, 2023 13:47
@nden
Copy link
Collaborator

nden commented Aug 16, 2023

I suggest we add the code in assign_wcs_step, just before the SIP computation. It doesn't feel right to mix it with the computation of S_REGION.

@tapastro tapastro changed the title Add bounding_box to imaging WCS objects without one JP-3343: Add bounding_box to imaging WCS objects without one Aug 16, 2023
@tapastro
Copy link
Contributor Author

New regtest run here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/867/

Strange failure has disappeared from regtests.

I moved the bbox generation to right before the S_REGION calculations, because most of them check for bbox presence and mock one if it didn't exist. This isn't quite where @nden wanted it - I can move it to assign_wcs_step if that's still preferred.

@tapastro tapastro force-pushed the add-nrs-imaging-bbox branch from 5471ad9 to 9e24241 Compare August 16, 2023 17:56
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

Initially I was suggesting to assign a bounding_box to all imaging modes in assign_wcs_step. Looking at the current changes I realized this will not cover resampled products. Now I think your first solution is best. Sorry for the flip flop.

@tapastro tapastro force-pushed the add-nrs-imaging-bbox branch from 34f18db to 4f9a351 Compare August 17, 2023 15:47
@tapastro tapastro marked this pull request as ready for review August 17, 2023 15:48
@tapastro tapastro added this to the Build 10.0 milestone Aug 17, 2023
@tapastro tapastro requested a review from nden August 17, 2023 17:05
@hbushouse
Copy link
Collaborator

Is everyone agreed that this is all OK now? @nden

@nden nden merged commit a3ba28b into spacetelescope:master Aug 19, 2023
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.

NIRSpec imaging products have no bounding box assigned to WCS
3 participants