Spring cleaning coordinate frames #457
Conversation
Cadair
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #457 +/- ##
==========================================
+ Coverage 87.42% 87.49% +0.06%
==========================================
Files 22 22
Lines 3874 3806 -68
==========================================
- Hits 3387 3330 -57
+ Misses 487 476 -11 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Cadair
force-pushed
the
visp_wcs
branch
2 times, most recently
from
cb649e2
to
ee6a91b
Compare
There was a problem hiding this comment.
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
force-pushed
the
visp_wcs
branch
5 times, most recently
from
217807e
to
88a39d0
Compare
|
I am going to split this up. |
Cadair
force-pushed
the
visp_wcs
branch
3 times, most recently
from
96f0464
to
e0aa648
Compare
Cadair
force-pushed
the
visp_wcs
branch
3 times, most recently
from
4849e01
to
846b0cf
Compare
This means that world_axis_object_components can be automatically sorted for all frames.
It's done by call_forward/backward
Seems this leaves one troublesome test
This means that _prop is in native order and is sorted into axes order
Also add some useful env vars
This adds back a more sane equivalent of coordinates and coordinates_to_quantity.
WilliamJamieson
force-pushed
the
visp_wcs
branch
from
19d024b
to
3bcf81e
Compare
|
I think we should make a choice on this question before we merge: #457 (comment) |
| @pytest.fixture | ||
| def gwcs_with_pipeline_celestial(): | ||
| input_frame = cf.CoordinateFrame(2, ["PIXEL"]*2, | ||
| axes_order=list(range(2)), | ||
| unit=[u.pix]*2, | ||
| name="input") | ||
|
|
||
| spatial = models.Multiply(20*u.arcsec/u.pix) & models.Multiply(15*u.deg/u.pix) | ||
|
|
||
| celestial_frame = cf.CelestialFrame(axes_order=(0, 1), unit=(u.arcsec, u.deg), | ||
| reference_frame=coord.ICRS(), name="celestial") | ||
|
|
||
| custom = models.Shift(1*u.deg) & models.Shift(2*u.deg) | ||
|
|
||
| output_frame = cf.CoordinateFrame(2, ["CUSTOM"]*2, | ||
| axes_order=list(range(2)), unit=[u.arcsec]*2, name="output") | ||
|
|
||
| pipeline = [ | ||
| (input_frame, spatial), | ||
| (celestial_frame, custom), | ||
| (output_frame, None), | ||
| ] | ||
|
|
||
| return wcs.WCS(pipeline) |
There was a problem hiding this comment.
Could you move this as a function to the examples.py and add the fixture to the conftest.py?
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_orderand 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
CelestialFramewhere the native order is lon, lat. The__init__for frames is always in the native order, and a different effective order can be given usingaxes_order. The native ordering of a frame is then passed into aFramePropertiesclass, and properties on the frame itself return an ordered view into this class. This functionality is tested hereThis split between native and axes_order ordering is also used by
CompositeFrameto allow it to combine split axes ordering into the expected order. This is tested hereHigh 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_unitscontrols the return type of__call__inverseandtransform, ifwith_units=Truethen 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_forwardand_call_backwardmethods.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_quantityproperty on theModelthe units will be retained, added or stripped as needed. All this happens in the new_call_forwardand_call_backwardmethods.Finally, to align with the new
_call_backwardmethodWCS.numerical_inverseis 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 byinverse. (with_units=Truetonumerical_inversenow 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
HighLevelWCSMixinThe
GWCSAPIMixinnow inherits from theHighLevelWCSMixinand 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_valuesandvalues_to_high_level_objectsfromastropy.wcs.wcsapi.high_level_apito convert to/from high level objects when they are passed to the user facing methods or to return them from these methods whenwith_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
coordinatesandcoordinate_to_quantitymethods have been removed.Breaking changes:
WCS.numerical_inverseno longer handles high level inputs or outputs,with_units=Trueerrors.CoordinateFrame.coordinatesand.coordinate_to_quantityhave been removed, these should never really have been used by users.CelestialFrameshould always be in lon,lat order and will be re-ordered by axes_order. This behaviour before was poorly specified and inconsistent.Open Questions
RegionSelector seems to not propagateuses_quantitycorrectly: Spring cleaning coordinate frames #457 (comment)fixes #224
hopefully fixes #269