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 2 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 76.10%. Comparing base (088212b) to head (6144111).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
- Coverage   76.21%   76.10%   -0.11%     
==========================================
  Files         115      115              
  Lines        7631     7639       +8     
==========================================
- Hits         5816     5814       -2     
- Misses       1815     1825      +10     

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WCS does not roundtrip
3 participants