Spring cleaning coordinate frames

[Cadair]

This PR has been going on for a long time, but I think it's finally close. So here's a write up of what this PR does and why based on what I can remember and my own review of the code.

Objective

Ultimately this whole thing span out of trying to fix #455, which boils down to axes_order and the way frames worked being inconsistent and weird. So the primary objective of this PR is to clarify, fix and document the purpose of the frames and how they interact with the WCS class and WCS API.

Core changes

Coordinate Frame Ordering

The coordinate frames now have a "native" order of axes, the main one where this is apparent is CelestialFrame where the native order is lon, lat. The __init__ for frames is always in the native order, and a different effective order can be given using axes_order. The native ordering of a frame is then passed into a FrameProperties class, and properties on the frame itself return an ordered view into this class. This functionality is tested here

This split between native and axes_order ordering is also used by CompositeFrame to allow it to combine split axes ordering into the expected order. This is tested here

High Level Objects and Units

The other main change is to the handling of units and high level objects. This has a few smaller prongs to it.

Firstly, this PR codifies that with_units controls the return type of __call__ inverse and transform, if with_units=True then high level astropy objects (Quantity, SkyCoord, Time etc) will be returned. All high level object handling is now confined to these user facing methods, and numerical only (Quantity or non-Quantity) transforms are executed by the new _call_forward and _call_backward methods.

Secondly, this PR also codifies and refactors the input handling for all these methods. Quantity and non-Quantity inputs are always supported for forward transforms, and depending on the transform.uses_quantity property on the Model the units will be retained, added or stripped as needed. All this happens in the new _call_forward and _call_backward methods.

Finally, to align with the new _call_backward method WCS.numerical_inverse is now low-level only, it can no longer accept high-level objects to be type converted by the frames, and will not return them either. If you need this functionality it will be automatically handled by inverse. (with_units=True to numerical_inverse now raises an exception).

Use more high level object handling from astropy core

The final major change in this PR is to remove as much high level WCS API code as possible and rely on astropy to do this for us. This consists of two main parts:

Using HighLevelWCSMixin

The GWCSAPIMixin now inherits from the HighLevelWCSMixin and no longer implements the high level API as this is provided by the mixin.

Removing custom high <> low level object conversion

We now use high_level_objects_to_values and values_to_high_level_objects from astropy.wcs.wcsapi.high_level_api to convert to/from high level objects when they are passed to the user facing methods or to return them from these methods when with_units=True.

The combination of these two changes means that we no longer need to implement low-level <> high-level conversions on the coordinate frame classes, so the coordinates and coordinate_to_quantity methods have been removed.

---

Breaking changes:

• WCS.numerical_inverse no longer handles high level inputs or outputs, with_units=True errors.
• CoordinateFrame.coordinates and .coordinate_to_quantity have been removed, these should never really have been used by users.
• Inputs to CelestialFrame should always be in lon,lat order and will be re-ordered by axes_order. This behaviour before was poorly specified and inconsistent.

Open Questions

1. High Level Object Input: Spring cleaning coordinate frames #457 (comment)
2. RegionSelector seems to not propagate uses_quantity correctly: Spring cleaning coordinate frames #457 (comment)

---

fixes #224
hopefully fixes #269

[codecov]

Codecov Report

Attention: Patch coverage is 94.98069% with 13 lines in your changes missing coverage. Please review.

Project coverage is 87.26%. Comparing base (eb9d316) to head (c037a05).
Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
gwcs/wcs.py	87.50%	8 Missing ⚠️
gwcs/coordinate_frames.py	98.19%	3 Missing ⚠️
gwcs/selector.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
- Coverage   87.28%   87.26%   -0.02%     
==========================================
  Files          22       21       -1     
  Lines        3821     3706     -115     
==========================================
- Hits         3335     3234     -101     
+ Misses        486      472      -14     

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

[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?

[Cadair]

I am going to split this up.

[Cadair]

I am not sure if this will always do the right thing here, this is very much built for pixel_to_world in the APE 14 parlance, so it might get confused by intermediate frames or world_to_pixel

[Cadair]

Note that we don't do this in self.transform for backward transforms.

[Cadair]

Question

inverse and transform need to decide how to handle high-level inputs and what with_units does for the output in these cases. Should we only accept high-level inputs if with_units=True and then also return high-level objects (i.e. Quantity) in that case. This would simplify the logic and make the input types explicit based on a kwarg. Currently inverse tries to guess but transform does not

[Cadair]

I have merged #512 into this as it was needed to fix some tests.

[Cadair]

I think other than the open threads this is now complete.

The fails in the jwst downstream tests look to be expected (one is the removal of .coordinates, and two are an invalid use of world_to_pixel [it should be world_to_pixel_values]).

I have tweaked the docs, and made all the other tests pass.