Repository navigation
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@cmarmo - thanks, that's quick! I have some initial comments, but also a request, to perhaps break this up in smaller PRs. This mostly since some parts I am confident are fine & needed, but I'm wondering a bit about the implementation of BodyLocation -- because of something I only recalled when I saw your PR, viz., that we've been thinking for quite a while that EarthLocation really shouldn't be a subclass of Quantity (see #4261). Indeed, the implementation of the various representations in #11086 by @astrojuanlu was meant to be a first step towards removing that.
Now I want to be sure that we don't let uncertainty about that stand in the way of actually getting a new BodyLocation implemented, but I think much will become clearer if we do it in steps. And that has the additional advantage that it makes review easier!
So, how about the following order:
- Separate out
astropy.unitssupport forgc2gdeandgd2gce, adding a few basic tests (I'd renametest_structured_erfa_ufunctotest_quantity_erfa_ufuncand add a classTestGeodetic). - Add support for
equatorial_radiusandflatteninginBaseGeodeticRepresentationand insert a by-ID subclass from which the earth-based ones inherit (see in-line comment). Could be either in currentearth.pyor a newgeodetic.py-- I think the latter would move underrepresentationsonce that is refactored to be a module with multiple files (see #13688). Can add tests totest_geodetic_representations. - Actually implement
BodyLocation.
p.s. If it makes your life a lot easier, fine to combine 1+2 - might save you writing specific erfa tests...
p.s. @astrojuanlu - if you have time to look at this too, it would be appreciated!
| """Convert to a tuple with X, Y, and Z as quantities.""" | ||
| return (self.x, self.y, self.z) | ||
|
|
||
| def get_itrs(self, obstime=None): |
There was a problem hiding this comment.
This should be on EarthLocation - the T is for Terristrial!
There was a problem hiding this comment.
uhh... 😱 ... shame on me! 🤦
|
|
||
| def __init_subclass__(cls, **kwargs): | ||
| super().__init_subclass__(**kwargs) | ||
| if "_ellipsoid" in cls.__dict__: |
There was a problem hiding this comment.
My sense is for the real base class to have _equatorial_radius and _flattening and then have a subclass of that (say, NamedGeodeticRepresentation??), which has ellipsoid and inside sets _equatorial_radius and _flattening using erfa.eform (maybe bform later!) - inside its own __init_subclass__() before calling super() to this part. That will make the implementation below considerably simpler (always using gc2gd2 and gd2gce and thus removing the need for gc2gd and gd2gc.
| def helper_gc2gde(f, unit1, nounit, unit2): | ||
| from astropy.units.si import m, radian | ||
|
|
||
| if nounit is not None: |
There was a problem hiding this comment.
I think that's too strict: it should just be dimensionless_unscaled (as a ratio of two radii). Note that this allows putting in float/ndarray - they will be treated as dimensionless. So this can just go inside the try/except below.
There was a problem hiding this comment.
Addressed in #14729, I wonder if the error message there will benefit from being more explicit though...
| def helper_gd2gce(f, unit1, nounit, unit2, unit3, unit4): | ||
| from astropy.units.si import m, radian | ||
|
|
||
| if nounit is not None: |
| Converts geodetic coordinates to 3D rectangular (geocentric) | ||
| cartesian coordinates. | ||
| """ | ||
| if hasattr(self, "_flattening"): |
There was a problem hiding this comment.
If we follow my suggestion above, then to_cartesian will only need this branch, since flattening and equatorial_radius will always be defined.
| Converts 3D rectangular cartesian coordinates (assumed geocentric) to | ||
| geodetic coordinates. | ||
| """ | ||
| if hasattr(cls, "_flattening"): |
There was a problem hiding this comment.
Same here, no need for the _ellipsoid branch.
|
Thanks for the ping @mhvk, I'm glad that there is steady progress towards better support for other bodies in Astropy! Unfortunately I'm not using the library on a daily basis anymore and it's unlikely that I'll have time to review this PR. |
…sentation. Add tests and documentation.
|
I am going to close this pr as we are moving with a step by step strategy ... and I was already messing up with the branches.... 😅 |
Description
Towards #11170.
This pull request refactors
EarthLocationas instance ofBodyLocation.The class
BaseGeodeticRepresentationcan now be described using customequatorial_radiusandflatteningparameters.Geodetic computations are then performed calling
erfa.gc2gdeanderfa.gd2gcewhen no standard ellipsoid is available.Helpers for those two functions have been added.
This work is funded by the Europlanet 2024 Research Infrastructure (RI) Grant.