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
Spring cleaning coordinate frames #457
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
cb649e2
to
ee6a91b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
||
# 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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this onto the WCS
class proper and not the mixin.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods are now provided by the HighLevelWCSMixin
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing the generated transform to have a mix of using Quantity and not using it, this makes it all not-quantity.
else: | ||
return result | ||
|
||
def numerical_inverse(self, *args, **kwargs): | ||
def numerical_inverse(self, *args, tolerance=1e-5, maxiter=50, adaptive=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified this to be numerical only, and then plan to make invert
handle high-level objects.
217807e
to
88a39d0
Compare
I am going to split this up. |
96f0464
to
e0aa648
Compare
4849e01
to
846b0cf
Compare
f034b90
to
f4332ae
Compare
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.
@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 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. |
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