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

Refactor axis_physical_types #459

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Conversation

Cadair
Copy link
Collaborator

@Cadair Cadair commented Jun 20, 2023

This is the second part of #457

The main work here is refactoring the logic of deciding the default coordinate frames from the base class into subclasses. Also add a PixelFrame which supports multiple dimensions. I haven't done anything with Frame2D here, but imo this is a complete replacement for it?

@Cadair Cadair requested a review from a team as a code owner June 20, 2023 14:28
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 97.77% and project coverage change: -0.35 ⚠️

Comparison is base (8ded81c) 87.25% compared to head (1c20498) 86.90%.

❗ Current head 1c20498 differs from pull request most recent head b61372d. Consider uploading reports for the commit b61372d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   87.25%   86.90%   -0.35%     
==========================================
  Files          23       23              
  Lines        3820     3857      +37     
==========================================
+ Hits         3333     3352      +19     
- Misses        487      505      +18     
Impacted Files Coverage Δ
gwcs/coordinate_frames.py 95.26% <97.77%> (-0.12%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nden
Copy link
Collaborator

nden commented Jun 21, 2023

These changes look good to me overall. It's unclear what the return type should be. In the FITS implementation it's defined as iterable and the examples return a list. GWCS uses tuples. How important is to be consistent т тхат лежел?

The new PixelFrame does not add much, however if it's convenient, I'm fine with adding it. I should note that there are no tests with this PR. And the new frame will need a Converter.
JWST regression tests pass: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/750/

@Cadair Cadair force-pushed the frame_clean_up_1 branch from 1c20498 to eadae51 Compare June 23, 2023 10:17
This moves the logic for the default physical types into the subclasses
rather than the base class.
@Cadair Cadair force-pushed the frame_clean_up_1 branch from eadae51 to b61372d Compare June 23, 2023 10:19
@Cadair
Copy link
Collaborator Author

Cadair commented Jun 23, 2023

I would say tuple rather than a list is perfectly fine, iterable is iterable. 🤷

@Cadair
Copy link
Collaborator Author

Cadair commented Jun 23, 2023

I have pulled the PixelFrame commit from this PR, will add it to its own one.

@Cadair Cadair changed the title Refactor axis_physical_types and add PixelFrame Refactor axis_physical_types Jun 23, 2023
@nden
Copy link
Collaborator

nden commented Jun 25, 2023

Failure is unrelated

@nden
Copy link
Collaborator

nden commented Jun 26, 2023

@Cadair I'm trying to sort out the jwst regression tests and once done will merge this.

@nden nden merged commit 54e6e12 into spacetelescope:master Jun 26, 2023
@nden nden added this to the 0.19 milestone Jun 26, 2023
@Cadair Cadair deleted the frame_clean_up_1 branch June 27, 2023 07:58
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.

2 participants