Spring cleaning coordinate frames #457

Cadair · 2023-06-15T13:32:46Z

This started as a project to solve #455 but ended up on a whole quest into coordinate_frames.py. This PR might get quite large, but I am going to try and keep the commit history very clean so it can be reviewed commit-by-commit or easily broken up into multiple PRs.

I have a whole bunch of questions, which I will call out with inline comments.

fixes #224 (by deleting it)
hopefully fixes #269

gwcs/coordinate_frames.py

codecov · 2023-06-15T13:35:36Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
gwcs/api.py	`95.49% <100.00%> (-1.18%)`	⬇️
gwcs/converters/wcs.py	`84.07% <ø> (-8.43%)`	⬇️
gwcs/utils.py	`86.32% <100.00%> (-0.04%)`	⬇️
gwcs/wcstools.py	`80.30% <100.00%> (ø)`
gwcs/coordinate_frames.py	`89.85% <95.96%> (-5.61%)`	⬇️
gwcs/wcs.py	`57.23% <84.48%> (-31.56%)`	⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

gwcs/coordinate_frames.py

Cadair

This is still very WIP.

For today, I have been focusing on what is needed to remove the CoordinateFrame methods coordinates() and coordinate_to_quantity as we should be able to use the Astropy wcsapi infrastructure to do those conversions for us now.

To this end, I started cleaning up the transform APIs (as users of those methods). I have added private _call_forward and _call_backward methods which only handle the old with_units=False case.

There appear to be multiple inconsistencies in how Quantity & high level objects were handled in various places around the API, it seems we were a lot more permissive than the docs stated, and would convert pretty much anything. I have maintained the ability to call the transforms with or without quantity irrespective of if the underlying model uses quantity or not.

After all this I have a question: There is clearly need to keep a more flexible API than the APE 14 API to handle kwargs being passed through to the transform or to control the numerical inverse. Should we continue to use with_units= as a way to differentiate the high and low level object inputs/outputs, or should we have separate methods like APE 14?

docs/index.rst

Cadair · 2023-06-19T15:43:51Z

gwcs/api.py


        # If we only have one output axes, we shouldn't return a tuple.
        if self.output_frame.naxes == 1 and isinstance(result, tuple):
            return result[0]
        return result

-    def _add_units_input(self, arrays, transform, frame):


I moved this onto the WCS class proper and not the mixin.

Cadair · 2023-06-19T15:44:19Z

gwcs/api.py

@@ -270,73 +262,6 @@ def world_axis_object_classes(self):
    def world_axis_object_components(self):
        return self.output_frame._world_axis_object_components

-    # High level APE 14 API


These methods are now provided by the HighLevelWCSMixin

Cadair · 2023-06-19T15:45:02Z

gwcs/wcstools.py

@@ -302,7 +302,7 @@ def wcs_from_points(xy, world_coords, proj_point='center',
                         "Only one of {} is supported.".format(polynomial_type,
                                                               supported_poly_types.keys()))

-    skyrot = models.RotateCelestial2Native(crval[0], crval[1], 180*u.deg)
+    skyrot = models.RotateCelestial2Native(crval[0].to_value(u.deg), crval[1].to_value(u.deg), 180)


This was causing the generated transform to have a mix of using Quantity and not using it, this makes it all not-quantity.

Cadair · 2023-06-19T15:45:58Z

gwcs/wcs.py

        else:
            return result

-    def numerical_inverse(self, *args, **kwargs):
+    def numerical_inverse(self, *args, tolerance=1e-5, maxiter=50, adaptive=True,


I have modified this to be numerical only, and then plan to make invert handle high-level objects.

Cadair · 2023-06-20T13:18:49Z

I am going to split this up.

gwcs/tests/test_wcs.py

gwcs/coordinate_frames.py

This creates a `BaseCoordinateFrame` class, definining the minimal API for a coordinate frame with descriptive docstrings. This is done mainly as an exercise to easily review and document the API. Also add significant docstring to the module describing how and why coordinate frames work.

The goal of this refactoring is to be able to remove `Frame.coordinates` and `Frame.coordinate_to_quantity` and rely on the Astropy WCSAPI machinery to do those conversions.

coordinates() and coordinate_to_quantity() are replaced by APE 14 methods

This highlights some API changes here.

Closes spacetelescope#436

fixes spacetelescope#269

nden · 2023-11-21T09:00:10Z

@Cadair I took your branch and made some changes so tests pass in both cases - reordering axes on a single frame, as well as reordering axes on a composite frame. The changes are on this branch

https://github.com/nden/gwcs/tree/add-on-spring-cleaning

The only tests that fail now are the slicing tests, as well as the converters (because frame._prop is not serialized). While the converters can be adapted I am not sure about the slicing. I'm also not very familiar with the slicing code. Could you look at the changes and the failing tests

https://github.com/nden/gwcs/actions/runs/6941325073/job/18881940341

to see if the current design will work with slicing or if we need to change anything.

I can make a PR to this one if you think it's easier to see the changes.

Cadair changed the title ~~Spring Cleaning coordinate frames~~ Spring cleaning coordinate frames Jun 15, 2023

Cadair commented Jun 15, 2023

View reviewed changes

gwcs/coordinate_frames.py Show resolved Hide resolved

Cadair commented Jun 15, 2023

View reviewed changes

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

Cadair force-pushed the visp_wcs branch 2 times, most recently from cb649e2 to ee6a91b Compare June 15, 2023 15:35

Cadair commented Jun 19, 2023

View reviewed changes

Cadair force-pushed the visp_wcs branch 5 times, most recently from 217807e to 88a39d0 Compare June 20, 2023 13:17

Cadair closed this Jun 20, 2023

Cadair reopened this Jun 20, 2023

Cadair force-pushed the visp_wcs branch from 88a39d0 to a9bbf19 Compare June 20, 2023 14:25

This was referenced Jun 20, 2023

Remove old bbox support #458

Merged

Refactor axis_physical_types #459

Merged

More refactoring #460

Merged

Cadair force-pushed the visp_wcs branch 3 times, most recently from 96f0464 to e0aa648 Compare June 20, 2023 15:55

Cadair commented Jun 20, 2023

View reviewed changes

gwcs/tests/test_wcs.py Show resolved Hide resolved

Cadair force-pushed the visp_wcs branch 3 times, most recently from 4849e01 to 846b0cf Compare June 23, 2023 11:11

Cadair force-pushed the visp_wcs branch from 846b0cf to e985768 Compare July 3, 2023 14:14

nden force-pushed the visp_wcs branch from e985768 to b93aa21 Compare July 5, 2023 13:18

Cadair commented Jul 5, 2023

View reviewed changes

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

Cadair mentioned this pull request Aug 7, 2023

Allow splitting Celestial axes #467

Closed

Cadair commented Oct 12, 2023

View reviewed changes

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

Cadair commented Oct 12, 2023

View reviewed changes

gwcs/coordinate_frames.py Outdated Show resolved Hide resolved

Cadair force-pushed the visp_wcs branch 2 times, most recently from f034b90 to f4332ae Compare October 12, 2023 21:12

Cadair added 15 commits October 12, 2023 17:14

First pass at restructuring the pixel <> world API

870286d

The goal of this refactoring is to be able to remove `Frame.coordinates` and `Frame.coordinate_to_quantity` and rely on the Astropy WCSAPI machinery to do those conversions.

Remove now unused methods

6ccdfe0

coordinates() and coordinate_to_quantity() are replaced by APE 14 methods

Rewrite coordinate systems tests for APE 14

8e333f2

This highlights some API changes here.

cleanup

74b88fa

Revert changes to inverse w/r/t with_units

dc717ba

Use is_high_level to detect input to inverse

a199c43

Remove isnumerical

7694b11

Remove old test

78af31e

Remove unused imports

5ae6f4c

Fix doc build

46b07cf

Delete reference_position

ef836af

Closes spacetelescope#436

lint

1b4a338

Add a test for axes ordering with CelestialFrame

8026e87

fixes spacetelescope#269

Update gwcs/coordinate_frames.py

6ec2d06

Cadair force-pushed the visp_wcs branch from f4332ae to 6ec2d06 Compare October 12, 2023 21:14

Cadair mentioned this pull request Oct 12, 2023

Deprecate the pre-APE 14 API #456

Closed

Cadair added 3 commits October 12, 2023 17:25

even more lint

50e2340

Test different Units said Nadia 💥

2e33bb2

First attempt at keeping a sorted and unsorted list of frame props

e65454e

Cadair mentioned this pull request Oct 16, 2023

Coordinate fixes DKISTDC/dkist#305

Merged

SolarDrew mentioned this pull request Nov 8, 2023

Tests for Dataset.crop() DKISTDC/dkist#310

Draft

nden mentioned this pull request Dec 1, 2023

Add on spring cleaning #486

Open

braingram mentioned this pull request Mar 11, 2024

bump gwcs version check in test DKISTDC/dkist#346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spring cleaning coordinate frames #457

Spring cleaning coordinate frames #457

Cadair commented Jun 15, 2023 •

edited

codecov bot commented Jun 15, 2023 •

edited

Cadair left a comment

Cadair Jun 19, 2023

Cadair Jun 19, 2023

Cadair Jun 19, 2023

Cadair Jun 19, 2023

Cadair commented Jun 20, 2023

nden commented Nov 21, 2023

Spring cleaning coordinate frames #457

Are you sure you want to change the base?

Spring cleaning coordinate frames #457

Conversation

Cadair commented Jun 15, 2023 • edited

codecov bot commented Jun 15, 2023 • edited

Codecov Report

Cadair left a comment

Choose a reason for hiding this comment

Cadair Jun 19, 2023

Choose a reason for hiding this comment

Cadair Jun 19, 2023

Choose a reason for hiding this comment

Cadair Jun 19, 2023

Choose a reason for hiding this comment

Cadair Jun 19, 2023

Choose a reason for hiding this comment

Cadair commented Jun 20, 2023

nden commented Nov 21, 2023

Cadair commented Jun 15, 2023 •

edited

codecov bot commented Jun 15, 2023 •

edited