Repository navigation
Modifications to transforming frames with data as opposed to SkyCoord - #10475
Conversation
pllim
added
the
API change
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This looks very good, the solution feels very much right! One very minor suggestion for the warning text. Other than that, I think
https://docs.astropy.org/en/latest/coordinates/frames.html#transforming-between-frames
has one more instance of using an uninstantiated class (possibly the cause of the CI failure?)
More generally, perhaps that particular page could do with a slight rewrite nearer the top, where it might suggest more strongly that SkyCoord is the way to go and that the page is mainly about defining new coordinate systems. But overall the documentation is actually quite clear that one should use SkyCoord, so this is not critical for this PR.
Beyond that, this will obviously need entries in CHANGES.rst, in particular about the API change!
@adrn, @eteq - this PR makes changes that I think you should have input on. See the linked PR on top for an earlier attempt and discussion, as well as @StuartLittlefair's suggestion to go this route (which @ayshih and I agreed with).
| def test_transform_to(): | ||
| for frame in (FK5, FK5(equinox=Time('J1975.0')), | ||
| FK4, FK4(equinox=Time('J1975.0')), | ||
| for frame in (FK5(), FK5(equinox=Time('J1975.0')), |
There was a problem hiding this comment.
This is a test where it is truly obvious this is an improvement!
|
As I was adding the empty parentheses to explicitly instantiate destination frames, I amusingly discovered that there was a smattering of occurrences of empty parentheses already being used, possibly because the writer wanted to make explicit what the destination frame was. |
|
I don't know if other packages would find these changes to be disruptive, but at least for SunPy, only ten lines need to be fixed. |
| "explicitly instantiate the target frame, or first convert the source " | ||
| "frame instance to a `astropy.coordinates.SkyCoord` and use its " | ||
| "`transform_to()` method.", | ||
| AstropyDeprecationWarning) |
There was a problem hiding this comment.
Good! Let's also remember to make an issue to remove this warning and functionality.
|
Okay, PR updated:
|
|
I just realized that this PR hadn't been merged yet... I've now rebased it against the latest. |
adrn
left a comment
adrn
left a comment
There was a problem hiding this comment.
The travis failure is unrelated, so I think this is ready to merge!
|
Thanks very much @ayshih ! |
|
This code in Ginga (modified so you can run it outside of Ginga) now raises deprecation warning with astropy dev. import numpy as np
from astropy import coordinates, units
coordsys = 'fk5'
system = 'icrs'
wcspt = np.array([[0.1234, -34.34], [0.4567, 42.42]])
frame_class = coordinates.frame_transform_graph.lookup_name(coordsys)
ra_deg = wcspt[:, 0]
dec_deg = wcspt[:, 1]
coord = frame_class(ra_deg * units.degree, dec_deg * units.degree)
to_class = coordinates.frame_transform_graph.lookup_name(system)
coord = coord.transform_to(to_class) # Deprecation warning hereIs this the correct solution? coord = coord.transform_to(to_class()) |
|
Yes, that is correct! The idea was that down at the frame level one should be specific. |
The coordinates transformation framework is currently a bit confusing/inconsistent because users can work with either
SkyCoordinstances or frame instances with data. Frame instances with data behave differently fromSkyCoordbecause they lack the ability to merge frame attributes across a transformation path. This is particularly confusing when transforming a frame instance with data to a frame class, because the behavior is significantly different from the seemingly analogous call when usingSkyCoord. See the discussion in #10453.This PR makes the following changes:
CoordinateAttributeso that it always usesSkyCoordfor the internal transformation (with the default setting ofmerge_attributes=True).SkyCoord, unless there is some strong reason to avoid usingSkyCoord.BaseCoordinateFrame.transform_to(), and point users toSkyCoord.To do:
CoordinateAttributeSkyCoord