Add basic example building Mars reference frames. by cmarmo · Pull Request #14990 · astropy/astropy

cmarmo · 2023-06-26T23:15:54Z

Description

This pull request addresses item 1 in #14820 (review).
The example about creating a reference frame for Mars is cleaned up (thanks @mhvk for your explanations, I was climbing the wrong mountain... 😕), the ITRS test is added.

I've kept the definition of a BaseBodyCoordinateFrame even if for now it looks unnecessary. Here's why:

I have listed some attributes (unused for now) which will be useful in the follow-up, hopefully for the implementation of the transformations between coordinate frames. The NAIF code, for example, is going to help in retrieving orbital info (perhaps reusing some machinery in solar_system.py). The observer position (as in sunpy), will be available in the next major version of WCSLIB.
Imagery from satellites (Jupiter's and Saturn's) are becoming common, I would like astropy providing an abstraction tool for the definition of new body frames rather than a hard-coded list of bodies which will need updates each time a new object is mapped.

DId I successfully make my case? 😇

As always, thanks for considering this pull request!

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

github-actions · 2023-06-26T23:16:30Z

pllim · 2023-06-27T13:30:08Z

+
+from .icrs import ICRS
+
+__all__ = ["BaseBodyCoordinateFrame"]


This is technically a new API, so this PR needs a change log.

cmarmo · 2023-06-28T18:40:32Z

For some reason my last commit is not taken into account ... I'm going to close and reopen the PR, to see if CI are triggered.

cmarmo · 2023-06-30T01:48:46Z

Still trying to justify the introduction of a base class ... I have added a North Pole coordinates attribute to the class. The ephemerids of the prime meridian should also be there but they depend on the observation time, so maybe a generic method to be overwritten would be better.... but it is beyond the scope of this pull request.
I would be more comfortable having a generic base class for the follow-up of planetary applications...

mhvk

@cmarmo - thanks for the attempt at splitting out and apologies for being a bit tardy.

I think this PR still introduces BaseBodyCoordinateFrame in a way that is unclear and would suggest to simplify it so we can just merge the example you made, and focus on how to implement the actual frame elsewhere. In particular, as per my previous comment, "this should not be in builtin_frames. In principle, if it stays really basic and adds a bit more than it does now to BaseCoordinateFrame, it could be in base_frame.py. Alternatively, it might make more sense in solar_system.py, including machinery to generate planet-specific frames on demand, though my sense is that perhaps for starters we should stick it in wcs."

The more I think about it, the better it seems to have the minimal version constructed on the spot in wcs, where we can just use what is needed, without yet worrying about how to transform to other systems.

cmarmo · 2023-07-05T19:54:37Z

@mhvk , I have applied your comments. As this PR no longer adds a new class I guess there is no need for a changelog?

mhvk

OK, looks good. Just one request to add a bit of clarification.

mhvk · 2023-07-05T20:34:06Z

+# representation with respect to the spherical model, assuming the point on the
+# surface of the body (``height = 0``)
+
+mars_sphere = MarsCoordinateFrame(lon=np.linspace(0, 2*np.pi, 128)*u.radian,


Since I am still asking for an edit, could you align the indentations while you are at it?

(Also, I'd do np.linspace(0, 360, 121) * u.deg - radians are just so annoying! but up to you.)

Hope this is what you had in mind...

mhvk

Yes, all good now!

mhvk

Sorry, I think we can improve the plot still a little -- I'll check at home...

mhvk · 2023-07-05T21:45:31Z

+mars_ocentric = MarsCoordinateFrame(
+    lon=np.linspace(0, 360, 128)*u.deg,
+    lat=np.linspace(-90, 90, 128)*u.deg,
+    representation_type=MarsBestFitOcentricAeroid)


Just to be sure, is this plot the one you expected?

is this plot the one you expected?

Yes, I checked the trends with lat and long and it seems to me that they are correct.... their readability could probably be improved...

OK. I wasn't quite sure how to read the figure, but after a bit more staring think I've got it. Not quite sure how to improve it, so maybe fine to leave it as is.

mhvk · 2023-07-05T21:47:57Z

+
+fig, ax = plt.subplots(2, subplot_kw={"projection": "3d"})
+
+ax[0].scatter(


Sorry to see at this only now, but in principle we should not use private properties and we should be able to use units... (using astropy.visualization.quantity_support()) I'll have a closer look tonight or tomorrow.

I'm checking the use of quantity_support and how to get rid of the private properties.

mhvk · 2023-07-05T22:42:51Z

@cmarmo - I couldn't push to your repository directly so made a PR to it - see oudeis-io#1

cmarmo · 2023-07-05T23:47:59Z

I couldn't push to your repository directly so made a PR to it - see oudeis-io#1

Sorry! I didn't check the PR before pushing new commits... I'm going to merge your pr after fixing the conflicts.

mhvk · 2023-07-05T23:54:43Z

I think the last 3 commits now do nothing -- you can check with git diff HEAD~3. If so, maybe just git rebase -i HEAD~6 and get rid of those?

cmarmo · 2023-07-05T23:56:21Z

Yes, rebasing and fixing pre-commit where I messed things up...

mhvk

Great! Will set this to be merged once tests pass.

github-actions Bot added coordinates Docs labels Jun 26, 2023

cmarmo mentioned this pull request Jun 26, 2023

Implement transformation to / from CoordinateFrame from / to related WCS #14820

Merged

pllim added this to the v6.0 milestone Jun 27, 2023

pllim reviewed Jun 27, 2023

View reviewed changes

cmarmo closed this Jun 28, 2023

cmarmo reopened this Jun 28, 2023

cmarmo force-pushed the basic-body-frame branch from f4cd096 to a096b98 Compare June 30, 2023 01:42

cmarmo force-pushed the basic-body-frame branch 2 times, most recently from 2640414 to e092916 Compare July 4, 2023 21:26

mhvk requested changes Jul 5, 2023

View reviewed changes

cmarmo force-pushed the basic-body-frame branch from e092916 to d329ef8 Compare July 5, 2023 19:41

cmarmo changed the title ~~Add basic body-fixed frame and Mars example.~~ Add basic example building Mars reference frames. Jul 5, 2023

mhvk reviewed Jul 5, 2023

View reviewed changes

mhvk approved these changes Jul 5, 2023

View reviewed changes

mhvk added Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed labels Jul 5, 2023

mhvk reviewed Jul 5, 2023

View reviewed changes

cmarmo force-pushed the basic-body-frame branch 2 times, most recently from 32ebc44 to e0dc584 Compare July 6, 2023 00:12

Add basic example building Mars reference frames.

e878d5a

cmarmo force-pushed the basic-body-frame branch from e0dc584 to e878d5a Compare July 6, 2023 00:14

mhvk approved these changes Jul 6, 2023

View reviewed changes

mhvk enabled auto-merge July 6, 2023 00:26

mhvk merged commit 2ba1afc into astropy:main Jul 6, 2023

cmarmo deleted the basic-body-frame branch July 6, 2023 01:26

cmarmo mentioned this pull request Jul 6, 2023

Planetary WCS implementation in Astropy #11170

Closed


		fig, ax = plt.subplots(2, subplot_kw={"projection": "3d"})

		ax[0].scatter(

Conversation

cmarmo commented Jun 26, 2023

Description

Uh oh!

github-actions Bot commented Jun 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmarmo commented Jun 28, 2023

Uh oh!

cmarmo commented Jun 30, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

cmarmo commented Jul 5, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

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!

mhvk commented Jul 5, 2023

Uh oh!

cmarmo commented Jul 5, 2023

Uh oh!

mhvk commented Jul 5, 2023

Uh oh!

cmarmo commented Jul 5, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants