Implement transformation to / from CoordinateFrame from / to related WCS by cmarmo · Pull Request #14820 · astropy/astropy

cmarmo · 2023-05-12T21:35:58Z

Description

This pull request Implement transformation to / from CoordinateFrame from / to related WCS.
This new frames are meant to be used for body-fixed reference frames for spheroidal Solar System bodies.
The implementation of the already available related WCS is also added.

Towards #11170.

Thanks for considering it.

This work is funded by the Europlanet 2024 Research Infrastructure (RI) Grant.

github-actions · 2023-05-12T21:36:37Z

nstarman · 2023-05-16T20:02:14Z

Hi @cmarmo, the actual PR looks pretty good, but I'm not sure I understand what it's meant to do... Is this a base class for use in custom frames outside of Astropy, or are you intending to refactor some existing frames in Astropy? If the former, I think this needs some examples demonstrating its usage; if the latter, this PR would be a good place to do the refactor 😄.

cmarmo · 2023-05-16T20:15:31Z

I'm not sure I understand what it's meant to do

It is meant to connect planetary WCS with a dedicated frame in astropy.
Still draft because I want to build the GeodeticRepresentation using the FITS keywords defined in the paper and I'm trying to figure out how to do it, as they are not in the WCS structure.
I was also preparing an example.

or are you intending to refactor some existing frames in Astropy

what kind of refactoring do you have in mind?

nstarman · 2023-05-16T20:19:09Z

what kind of refactoring do you have in mind?

I just saw this was a base class. If it was a base class for any existing frames, then that would be some small refactoring. If it's for new stuff, then carry on :)

mhvk · 2023-05-16T20:28:33Z

@nstarman - see #11170 for the larger picture. Thoughts on how to go about it very welcome -- we did the easy bits, which was to set up the option to create geodetic representations for other planets.

cmarmo · 2023-05-16T22:30:54Z

@nstarman I find the documentation about creating a custom frame quite detailed and usable with the base class introduced by this PR. I've added an example instead.
No transformations are defined for now, so the example is indeed minimal.

mhvk

@cmarmo - good to see the next step! I think we need to be a bit careful here not to mix up the representation (which you did already) with the frame. If I have coordinates on Mars, then the representation can be cartesian, spherical, geodetic or bodycentric, but the frame does not care about that. The only thing it needs to know is that it is of Mars, so that, if needed, it can be transformed to something relative to the solar system barycentre, etc. Basically, the frame should be like ITRS is for Earth, i.e., something suitable to add to the frame transformation graph. Of course, there should be different frames for the different planets, since they have different transformations to barycentric. (In princple, there might also be different frames for, say, different precession or orbital models, but that is definitely for another time!)

This has consequences both for the example plot in the new documentation (so good to add that, it helps focus discussion!) and for WCS.

For the example, there should be only one MarsCoordinateFrame. I don't know what the standard is exactly, but given that there are different spheroids, safest would seem to make the default Cartesian (as it is for Earth, via ITRS). And then one can initialize this with different geodetic and bodycentric representations, i.e.,the following would all work,

mars = MarsCoordinateFrame(x=1000*u.km, y=2000*u.km, z=3000*u.km)
mars2 = MarsCoordinateFrame(lon=10*u.deg, lat=20*u.deg, representation_type=MarsBestFitAeroid)

Indeed, note that even now, with the changes you have already made and with your definition of MarsBestFitAeroid in your example, one can do

mars = ITRS(x=1000*u.km, y=2000*u.km, z=3000*u.km)
mars.represent_as(MarsBestFitAeroid)  # or .represent_as("marsbestfitaeroid")
# <MarsBestFitAeroid (lon, lat, height) in (rad, rad, m)
#     (1.10714872, 0.93481117, 357668.0311648)>
mars2 = ITRS(lon=10*u.deg, lat=10*u.deg, representation_type=MarsBestFitAeroid)
mars2.cartesian
# <CartesianRepresentation (x, y, z) in m
#     (3293561.1722263, 580743.6972773, 583553.24056641)>

For WCS, this means that one should not instantiate a BodyBaseCoordinateFrame, but rather for each known object make a subclass, probably best on first use (i.e., cache the result), though perhaps it is fine to just define the 8 that we need in a dict (maybe replacing the one you already have).

One thing: if the default becomes Cartesian, there seems to be little point in having BodyBaseCoordinateFrame for now - it doesn't really do anything! One might as well add obstime attribute where needed.

Anyway, after this long comment, how about the following:

Make the documentation plot a separate PR that just subclasses BaseCoordinateFrame (and maybe includes the ITRS test; alternatively, rewrite that test to make a Mars frame as you do in the plot example - since those do not get tested, would be nice to have it!
Here, focus on WCS. I think creating custom classes may be the best way forward for now, but let me ping @mcara and @nden as WCS maintainers too.
Eventually, move class creation to coordinates, but then it would be good to actually link the frames to the transform graph (so, e.g., one could know where to point a telescope to observe Olympus Mons). Since we do have the machinery in place to calculate barycentric coordinates of the planets, this should not be very hard, but perhaps out of scope here.

Note that I'm not really sure whether or not to keep your BodyBaseCoordinateFrame - I mostly think in the end we'd like it to do more and I don't want to preclude that (also, it should probably be BaseBodyCoordinateFrame...). I'm also worried I'm just not thinking this through completely properly - in a way, these coordinates are like EarthLocation, which is very similar but not identical to ITRS. If we can avoid that can of worms...

mhvk · 2023-06-26T15:56:15Z

+            ycoord = "LAT-"
+        if frame.obstime is not None:
+            wcs.wcs.dateobs = frame.obstime.utc.isot
+        if issubclass(frame.representation_type, BaseGeodeticRepresentation):


In principle, these two if statements would also apply to ICRS, so perhaps there is a way to store that?

You mean ITRS, right?
I am confused about the ITRS WCS definition, because of the TLON TLAT descriptions which I cannot find in the literature... if someone has some references this will help me a lot... Thanks!

Indeed, I meant ITRS. I don't know where TLON, TLAT comes from either... But google gives, e.g., http://tdc-www.harvard.edu/software/wcstools/wcstools.multiwcs.html and https://www.astro.rug.nl/software/kapteyn/allsky.html, so I guess we should just continue to support it.

Thanks! I coudn't find those links.

cmarmo · 2023-06-29T22:30:04Z

I am converting this PR back to draft as #14990 needs to go in first.

cmarmo · 2023-07-06T05:21:35Z

Mark Calabretta has just released wcslib8.1, this pr is meant to work when built against wcslib versions >= 8.
May I offer a PR updating the shipped version of wcslib for astropy 6? And then I will find a way to check the wcslib version in this pr....

mcara · 2023-07-06T05:31:37Z

I'll make a PR to update bundled WCSLIB first thing in the morning. You can rebase this PR after that.

mcara · 2023-07-06T20:08:44Z

A PR to update WCSLIB was made here: #15035

Once it is approved, please rebase this PR.

Thanks!

cmarmo · 2023-09-18T20:20:27Z

I have addressed some of the comments ... looking forward to find some time to address the remaing ones... hopefully soon...

mhvk · 2023-09-18T20:25:51Z

Great! Do ping when all are done, and I'll re-review.

cmarmo · 2023-09-22T21:42:50Z

@mhvk I believe I have addressed your comments.
Thanks for your patience!

mhvk

Thanks! This seems almost done: the main code now has two unnecessary clauses, and I clarified what I meant with reorganizing the tests.

mhvk · 2023-09-23T14:12:27Z

+                equatorial_radius,
+                flattening,
+            )
+            if representation_type is None:


This whole clause is no longer needed - the above function call will either create the representation or return the cached function!

Indeed, this is why there is no coverage of these lines.

mhvk · 2023-09-23T14:12:58Z

+            frame = solar_system_body_frame(
+                SOLAR_SYSTEM_OBJ_DICT.get(xcoord[:2]), representation_type
+            )
+            if frame is None:


Same, this is no longer needed - the function call will do it.

mhvk · 2023-09-23T14:15:56Z

+        name = "Mars"
+        representation_type = SphericalRepresentation
+
+    frame = IAUMARSSphereFrame()


No, that is not what I meant: I just meant that in this function I feel it is better first to test things as they are supposed to work, and then at the end test that the right errors are raised. I.e., move this pytest.raises to the very end of the test function.

mhvk · 2023-09-23T14:17:10Z



+def test_wcs_to_body_frame():
+    unknown_wcs = WCS(naxis=2)


Like for the other direction, I would move the tests for failures to the very end of the test function. I think it is more helpful if a test function first shows how things are supposed to work, and then checks that you get an appropriate error if you do something incorrectly.

mhvk · 2023-09-23T14:17:27Z

+    ):
+        frame = wcs_to_celestial_frame(triaxial_wcs)
+
+    mywcs = WCS(naxis=2)


So, I would start the test function with this part.

mhvk · 2023-09-23T14:20:38Z

+    mywcs.wcs.aux.a_radius = 3396190.0
+    mywcs.wcs.aux.b_radius = 3396190.0
+    mywcs.wcs.aux.c_radius = 3376190.0
+    frame = wcs_to_celestial_frame(mywcs)


Maybe call this frame3, so that one has

frame3 = wcs_to_celestial_frame(mywcs) assert frame3.representation_type is not frame.representation_type assert frame3 is not frame

cmarmo · 2023-09-23T19:41:40Z

Better now? 😁

mhvk

Looks all great now! Let's get it in!

mhvk · 2023-09-24T07:42:33Z

@cmarmo - there's a bit of a what's-new already, but I realize now (after merging, of course, sigh) that it may be good to add a little to it, noting that WCS can also be written (with perhaps a link to the article describing the new standard). Perhaps add to the documentation too, if you think that is already useful.

cmarmo · 2023-09-24T09:46:50Z

@cmarmo - there's a bit of a what's-new already, but I realize now (after merging, of course, sigh) that it may be good to add a little to it, noting that WCS can also be written (with perhaps a link to the article describing the new standard). Perhaps add to the documentation too, if you think that is already useful.

PR coming soon.
Thanks a lot for your time and your patience @mhvk !

pllim · 2023-09-25T16:24:27Z

Does this have anything to do with this pyinstaller failure I started seeing?

https://github.com/astropy/astropy/actions/runs/6294343966/job/17086053141

mhvk · 2023-09-25T17:28:08Z

@pllim - I fear so. I think the combination of importing and setup/teardown just become too much for pytest to handle. Since really it does not matter exactly what is used in the tests, I just duplicated some definitions in #15381. Hopefully, with that, things will run (I used extra CI since I assume that's why we didn't notice the failures before).

cmarmo requested a review from mcara as a code owner May 12, 2023 21:35

cmarmo marked this pull request as draft May 12, 2023 21:36

github-actions Bot added coordinates wcs labels May 12, 2023

cmarmo mentioned this pull request May 12, 2023

Planetary WCS implementation in Astropy #11170

Closed

pllim added this to the v6.0 milestone May 12, 2023

cmarmo force-pushed the bodyfixed-frame branch from 4eb9f45 to 517efb2 Compare May 15, 2023 21:12

nstarman requested a review from mhvk May 16, 2023 20:04

cmarmo force-pushed the bodyfixed-frame branch from d449428 to 584e073 Compare May 22, 2023 19:11

cmarmo force-pushed the bodyfixed-frame branch from 584e073 to 8bfca9e Compare June 23, 2023 21:09

cmarmo marked this pull request as ready for review June 24, 2023 02:20

mhvk reviewed Jun 26, 2023

View reviewed changes

cmarmo mentioned this pull request Jun 26, 2023

Add basic example building Mars reference frames. #14990

Merged

cmarmo marked this pull request as draft June 29, 2023 22:29

cmarmo force-pushed the bodyfixed-frame branch 2 times, most recently from 5bfc67c to af7a6e1 Compare July 6, 2023 02:11

cmarmo changed the title ~~Implement basic BodyBaseCoordinateFrame and related WCS.~~ Implement transformation to / from CoordinateFrame from / to related WCS Jul 6, 2023

mcara mentioned this pull request Jul 6, 2023

Update WCSLIB to version 8.1 #15035

Merged

cmarmo force-pushed the bodyfixed-frame branch from af7a6e1 to 462093f Compare July 7, 2023 20:29

cmarmo marked this pull request as ready for review July 7, 2023 21:14

cmarmo added 10 commits September 22, 2023 22:37

Implement WCS related to solar system bodies.

9d5621c

Address comments.

06b0509

Preserve original REPRESENTATION_CLASSES dict.

ced5779

Address some comments.

cf213e0

Remove test on wcslib version and test with external dependencies.

d9de8f9

Fix code style

8caf574

Address some comments.

b6526bd

Fix tests.

beaf551

Fix lint.

0d9de08

Define lru cache functions.

9616bf6

cmarmo force-pushed the bodyfixed-frame branch from c2fefc7 to 9616bf6 Compare September 22, 2023 21:28

Revert ITRF with geodetic geocentric representations.

20332ff

mhvk reviewed Sep 23, 2023

View reviewed changes

Address comments.

d7815fd

mhvk approved these changes Sep 24, 2023

View reviewed changes

mhvk merged commit 4c160a0 into astropy:main Sep 24, 2023

cmarmo deleted the bodyfixed-frame branch September 24, 2023 09:46

cmarmo mentioned this pull request Sep 24, 2023

DOC Add what's new for planetary body-fixed WCS description. #15377

Merged

1 task

mhvk mentioned this pull request Sep 25, 2023

Avoid import of classes from another test file #15381

Merged

1 task

svank mentioned this pull request Nov 16, 2023

Custom WCS-frame mappings no longer supported? #15625

Closed

cmarmo mentioned this pull request Nov 20, 2023

Fix support of custom WCS mapping. #15630

Merged

1 task

mhvk mentioned this pull request May 26, 2025

Support several SPK ephemerides at once #6472

Open

Conversation

cmarmo commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented May 12, 2023 • edited by mhvk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented May 16, 2023

Uh oh!

cmarmo commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nstarman commented May 16, 2023

Uh oh!

mhvk commented May 16, 2023

Uh oh!

cmarmo commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmarmo commented Jun 29, 2023

Uh oh!

cmarmo commented Jul 6, 2023

Uh oh!

mcara commented Jul 6, 2023

Uh oh!

mcara commented Jul 6, 2023

Uh oh!

cmarmo commented Sep 18, 2023

Uh oh!

mhvk commented Sep 18, 2023

Uh oh!

cmarmo commented Sep 22, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmarmo commented Sep 23, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 24, 2023

Uh oh!

cmarmo commented Sep 24, 2023

Uh oh!

pllim commented Sep 25, 2023

cmarmo commented May 12, 2023 •

edited

Loading

github-actions Bot commented May 12, 2023 •

edited by mhvk

Loading

cmarmo commented May 16, 2023 •

edited

Loading

cmarmo commented May 16, 2023 •

edited

Loading