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

RCAL-856: WCS should roundtrip with sufficient accuracy #1273

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nden
Copy link
Collaborator

@nden nden commented Jun 13, 2024

Resolves RCAL-856

Closes #1272

This PR is the last one in a chain of PRs aiming to fix roundtripping of WCS transforms.
The only change here is to test data, ensuring the WCS is consistent and the inverse is wihtin the bounding box.

The order the PRs should be merged is

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@nden nden requested a review from a team as a code owner June 13, 2024 10:32
@nden nden marked this pull request as draft June 13, 2024 10:32
@github-actions github-actions bot added testing dependencies Pull requests that update a dependency file labels Jun 13, 2024
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.89%. Comparing base (e250097) to head (58c1477).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
- Coverage   78.00%   77.89%   -0.11%     
==========================================
  Files         115      115              
  Lines        7555     7563       +8     
==========================================
- Hits         5893     5891       -2     
- Misses       1662     1672      +10     

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

@nden nden force-pushed the fix-inverse-wcs-bbox branch from 8da6df5 to c4156a2 Compare June 13, 2024 11:02
Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

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

LGTM

@schlafly
Copy link
Collaborator

I think I missed the part of this test that checks that things round trip?

This WCS contains only the v2v3tosky bit, and not the CRDS distortion bit. That's good since it makes for an easier unit test but bad since it won't catch the only case we have had problems here so far, where the failure to round trip was in the CRDS distortion file. Do we intend to check the CRDS reference files as well?

I note that the bounding box is inconsistent with the size of the image, and that we're pretending that the sizes of the pixels are ~1". I think that's likely fine. romanisim has some code for making up a "fake" distortion function here
https://github.com/spacetelescope/romanisim/blob/main/romanisim/tests/test_wcs.py#L19-L38
and using that in unit tests instead of the actual CRDS distortion functions. It's not obvious to me that that is helpful here but I link it in case you would have rather used some kind of distortion function.

@schlafly
Copy link
Collaborator

schlafly commented Oct 2, 2024

@nden , do you plan to go forward with this in the future, or should we merge now?

@nden
Copy link
Collaborator Author

nden commented Oct 2, 2024

This requires a change in gwcs which I haven't had time to complete. I'll take it out of Draft when it's ready.

@nden nden force-pushed the fix-inverse-wcs-bbox branch from 77d1f40 to 7a58ca0 Compare October 29, 2024 14:00
@nden nden mentioned this pull request Oct 30, 2024
7 tasks
@nden nden force-pushed the fix-inverse-wcs-bbox branch from 7a58ca0 to 6144111 Compare November 8, 2024 14:49
@nden nden force-pushed the fix-inverse-wcs-bbox branch from 6144111 to f959e74 Compare December 10, 2024 21:53
@nden nden force-pushed the fix-inverse-wcs-bbox branch from fbd240f to 58c1477 Compare December 12, 2024 20:34
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.

WCS does not roundtrip
3 participants