Skip to content

Refactor Coordinate Frames#567

Merged
WilliamJamieson merged 7 commits into
spacetelescope:masterfrom
WilliamJamieson:refactor/coordinates
May 19, 2025
Merged

Refactor Coordinate Frames#567
WilliamJamieson merged 7 commits into
spacetelescope:masterfrom
WilliamJamieson:refactor/coordinates

Conversation

@WilliamJamieson

Copy link
Copy Markdown
Collaborator

This PR cleans up the coordinate_frames module, splitting it into submodules and fully implementing type hints to describe its API.

Note that this PR is built on #562 and #563 so those PRs should be merged first.

@codecov

codecov Bot commented Feb 24, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.47368% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.16%. Comparing base (f7128a3) to head (7af634b).
⚠️ Report is 69 commits behind head on master.

Files with missing lines Patch % Lines
gwcs/coordinate_frames/_empty.py 72.00% 14 Missing ⚠️
gwcs/coordinate_frames/_core.py 90.80% 8 Missing ⚠️
gwcs/coordinate_frames/_composite.py 93.24% 5 Missing ⚠️
gwcs/coordinate_frames/_properties.py 92.45% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   86.03%   86.16%   +0.13%     
==========================================
  Files          27       38      +11     
  Lines        4045     4092      +47     
==========================================
+ Hits         3480     3526      +46     
- Misses        565      566       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@WilliamJamieson
WilliamJamieson force-pushed the refactor/coordinates branch 2 times, most recently from be55069 to dfe79b4 Compare February 26, 2025 14:45
@WilliamJamieson
WilliamJamieson marked this pull request as ready for review May 14, 2025 18:47
@WilliamJamieson
WilliamJamieson requested a review from a team as a code owner May 14, 2025 18:47

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should this even be in coordinate_frames? It does not appear to be referenced anywhere inside it. It seems to actually be used only by the WCS so maybe move it into wcs._utils?

@mcara mcara May 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@nden could answer this better, but it seems that while it is not directly related to coordinate frames, ucd/ctype is important for axes types that are, in turn, related to the coordinate frames. However, it does not seem to me that the location of this is of much importance, especially if private.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@nden, do you have a preference?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't. Perhaps in the future there will be more need for coordinate_frames/utils but I can't think of anything right now. I'm fine if you want to move it.

Comment thread gwcs/coordinate_frames/__init__.py
__all__ = ["Frame2D"]


class Frame2D(CoordinateFrame):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I really should finish #461

@perrygreenfield perrygreenfield left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only a minor comment.

Comment thread gwcs/coordinate_frames/_celestial.py
@nden

nden commented May 19, 2025

Copy link
Copy Markdown
Collaborator

LGTM. Can you run the regression tests for both pipelines?

@WilliamJamieson

Copy link
Copy Markdown
Collaborator Author

The regression tests indeed pass.

@WilliamJamieson
WilliamJamieson merged commit 1a0b66c into spacetelescope:master May 19, 2025
@WilliamJamieson
WilliamJamieson deleted the refactor/coordinates branch May 19, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants