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

AL-837: Calculate output WCS for resample from already-known s_region #307

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

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Oct 16, 2024

Resolves AL-837

This PR simplifies the calculation of the output WCS for resample. Previously, the wcs_from_footprints function accepted a list of WCS objects, one for each input model, and computed the footprints from those WCS objects to pass into _calculate_new_wcs(). However, computing the footprints here is unnecessary because that information is already stored in the S_REGION keyword. This PR makes wcs_from_footprints take in an actual footprint instead, in the form of either a numpy array or an S_REGION string.

This PR will require small changes to both jwst and romancal, both of which are using the wcs_from_footprints function. I'm not sure there's a way to avoid that. One other non-hidden function changed, compute_fiducial, but this is not used for JWST or Romancal - both repositories have their own copy of a function with that name.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (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)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.24%. Comparing base (60bd3b8) to head (830cf47).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
+ Coverage   86.21%   86.24%   +0.02%     
==========================================
  Files          47       49       +2     
  Lines        8812     8838      +26     
==========================================
+ Hits         7597     7622      +25     
- Misses       1215     1216       +1     

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

@emolter
Copy link
Collaborator Author

emolter commented Oct 16, 2024

JWST regression tests started here

@emolter emolter marked this pull request as ready for review October 16, 2024 16:32
@emolter emolter requested a review from a team as a code owner October 16, 2024 16:32
@emolter emolter requested review from mairanteodoro and nden and removed request for a team October 16, 2024 16:32
@emolter
Copy link
Collaborator Author

emolter commented Oct 16, 2024

tracking down JWST regression test failures now

@mairanteodoro
Copy link
Collaborator

romancal's issue 1455 needs to be resolved before this PR, since it seems that TweakRegStep is only updating the WCS but not s_region.

@emolter
Copy link
Collaborator Author

emolter commented Oct 16, 2024

@mairanteodoro good to know. When you do update s_region, it might be good to be aware of this comment. The regression test failures for JWST come from the fact that wcs.footprint() and S_REGION differ by half a pixel due to JWST's use of center=True in compute_s_region_imaging()

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