Repository navigation
Add BaseBodycentricRepresentation - #14851
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.
|
d3f53fd to
a6e30fb
Compare
|
It seems like I'm still confused about units conversions... :( |
f1a8a58 to
25fb355
Compare
|
For the sake of completeness, this pull request makes the |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@cmarmo - Good to have these options, but I do think at least some of them should be subclasses rather than options on the base class. For _ellipsoid', I ended up agreeing it was not necessary, since having it on the baseclass gives a little overhead only at class creation time, and in the end it is really the same thing as far as the actual calculations are concerned. But here the calculations for conversion to/from cartesian are effectively separated, making the code more complicated and giving overhead for every calculation. Generally, I think each different interpretation of the coordinates should have its own (base) class -- just like PhysicsSpherical is a different class from Spherical. I would say the rule should be that the moment from_cartesian and to_cartesian depend on any class attribute, it would be better to have a different class.
So, I think we definitely should have a separate base class for the planetocentric latitudes (BasePlanetoCentric?). I think we also will be better off with a separate class for West longitudes, though given the rarity of its use, perhaps this is best implemented via a mixin class that overrides from_cartesian and to_cartesian changing the sign of the longitude appropriately.
Only for _wrap_angle, I think it is fine to leave that on the base class -- the calculations do not depend on it, one can see it as similar to changing the class of the attributes.
| to quantities holding correct values (with units of length and dimensionless, | ||
| respectively), or alternatively an ``_ellipsoid`` attribute to the relevant ERFA | ||
| index (as passed in to `erfa.eform`). | ||
| Longitudes are east positive and span from -180 to 180 degrees by default. |
There was a problem hiding this comment.
I think we should have separate classes; see main comment.
25fb355 to
50c29f4
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Thanks for making the change to a different base class. That does feel much better!
But, seeing this again, I wonder about the approach for wrap_angle. Really, the problem is that we have a Longitude class that has a default of 360 degrees, while we would like 180 degrees. Would it make sense to just define
class Longitude180(Longitude):
_default_wrap_angle = Angle(180 * u.deg)
and then use that? Then anyone who wants to have a different default, can just choose to use a different Longitude class. And we don't have to deal with the new _wrap_angle attribute.
p.s. Need to think a bit about the mixin still... And I haven't looked in detail over the tests.
| raise AttributeError( | ||
| f"{cls.__name__} requires '_ellipsoid' or '_equatorial_radius' and '_flattening'." | ||
| ) | ||
| if not hasattr( |
There was a problem hiding this comment.
Small detail, but could do
if not u.Quantity(cls._wrap_angle).unit.is_equivalent(u.deg):
(but see my larger comment about _wrap_angle)
|
Thanks for your review @mhvk ! |
cmarmo
changed the title
BaseGeodeticRepresentationBaseBodycentricRepresentation
|
The tests are failing because the class On the long term, I see the importance of changing Any suggestion? Thanks a lot! |
|
Simplest right now would seem to not allow |
d17d2dc to
265e9bb
Compare
|
Hi @mhvk . I believe I have removed all the unnecessary conversions to degrees, following your comments.
I guess this is also because I don't understand this previous comment |
|
@cmarmo - thanks for the changes. I'm still not sold on the However, I also think this discussion is actually distracting from the main point of this PR, which is to introduce a Indeed, while I think the |
I guess I'm loosing focus indeed... I need the Bodycentric representation for the frame definition, while west/east and 180/360 are not needed.... and I'm running out of time... ⏲️ |
|
OK, thanks, ping me when I should look again. And apologies for not realizing this earlier! |
No problem! I'm treasuring all the comments for the follow-up! :) |
317bc2b to
918a6b3
Compare
|
@mhvk thanks for your patience! |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This looks great! There are only nitpicks and suggestions for the docstrings left. But also: do indeed add to the whatsnew entry you already created for other geodetic representations.
| cls._equatorial_radius, cls._flattening, cart.get_xyz(xyz_axis=-1) | ||
| ) | ||
| return cls(lon, lat, height, copy=False) | ||
| return cls( |
There was a problem hiding this comment.
Nitpick, but could you get this back to the default format? Be sure to delete the final , after copy=False, otherwise black will break it over several lines again.
| to quantities holding correct values (with units of length and dimensionless, | ||
| respectively), or alternatively an ``_ellipsoid`` attribute to the relevant ERFA | ||
| index (as passed in to `erfa.eform`). | ||
| Longitudes are east positive and span from 0 to 360 degrees by default. |
There was a problem hiding this comment.
Nitpick: might as well leave this out for now.
Instead, I think here is the place to mention how lon, lat, and height are defined relative to the ellipsoid.
Alternatively, you could clarify the definition in geodetic_base_doc above (and then not use it for bodycentric)
| Subclasses need to set attributes ``_equatorial_radius`` and ``_flattening`` | ||
| to quantities holding correct values (with units of length and dimensionless, | ||
| respectively). | ||
| Longitudes are east positive and span from 0 to 360 degrees by default. |
There was a problem hiding this comment.
Again, I'd remove this for now and instead describe how the parameters are used (and how it is different from geodetic).
Alternatively, do not use geodetic_base_doc, but instead write a complete docstring here that also describes how lat is defined.
| lon, | ||
| lat, | ||
| height, | ||
| copy=False, |
There was a problem hiding this comment.
Again, could you bring this on a single line?
| @@ -0,0 +1,2 @@ | |||
| Add ``BaseBodycentricRepresentation``, a new spheroidal representation for bodycentric | |||
| latitudes and longitudes spanning from 0 to 360 degrees. | |||
There was a problem hiding this comment.
Let's remove the "spanning from 0 to 360 degrees" here too.
| `~astropy.coordinates.BaseBodycentricRepresentation` is the coordinate representation | ||
| recommended by the Cartographic Coordinates & Rotational Elements Working Group | ||
| (see for example its `2019 report <https://rdcu.be/b32WL>`_): the bodicentric latitude | ||
| and longitude are spherical latitude and longitude originated at the barycenter of the |
| degrees. | ||
| `~astropy.coordinates.BaseBodycentricRepresentation` is the coordinate representation | ||
| recommended by the Cartographic Coordinates & Rotational Elements Working Group | ||
| (see for example its `2019 report <https://rdcu.be/b32WL>`_): the bodicentric latitude |
| x_spheroid = self._equatorial_radius * coslat * coslon | ||
| y_spheroid = self._equatorial_radius * coslat * sinlon | ||
| z_spheroid = self._equatorial_radius * (1 - self._flattening) * sinlat |
There was a problem hiding this comment.
These variables are unused.
| x_spheroid = self._equatorial_radius * coslat * coslon | |
| y_spheroid = self._equatorial_radius * coslat * sinlon | |
| z_spheroid = self._equatorial_radius * (1 - self._flattening) * sinlat |
There was a problem hiding this comment.
Indeed! They are no longer needed as the calculation is done for r already.
| z_spheroid = self._equatorial_radius * (1 - self._flattening) * sinlat | ||
| r = ( | ||
| self._equatorial_radius | ||
| * np.sqrt(coslat**2 + ((1 - self._flattening) * sinlat) ** 2) |
There was a problem hiding this comment.
Could use np.hypot() here:
| * np.sqrt(coslat**2 + ((1 - self._flattening) * sinlat) ** 2) | |
| * np.hypot(coslat, (1 - self._flattening) * sinlat) |
| [ | ||
| WGS84GeodeticRepresentation, | ||
| IAUMARS2000BodycentricRepresentation, | ||
| ], |
There was a problem hiding this comment.
Doesn't this fit on one line?
| [ | |
| WGS84GeodeticRepresentation, | |
| IAUMARS2000BodycentricRepresentation, | |
| ], | |
| [WGS84GeodeticRepresentation, IAUMARS2000BodycentricRepresentation], |
If it does then the other similar cases should be updated analogously.
| degrees, the height is the elevation above the surface of the spheroid (measured on the | ||
| perpendicular to the surface). |
There was a problem hiding this comment.
Should this be replaced with the following?
| degrees, the height is the elevation above the surface of the spheroid (measured on the | |
| perpendicular to the surface). | |
| degrees, the height is the elevation above the surface of the spheroid (measured | |
| perpendicular to the surface). |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@cmarmo - only some nitpicks left beyond @eerovaher's comments. With those, I'll be happy to merge!
|
|
||
| Subclasses need to set attributes ``_equatorial_radius`` and ``_flattening`` | ||
| to quantities holding correct values (with units of length and dimensionless, | ||
| respectively). the bodicentric latitude and longitude are spherical latitude |
There was a problem hiding this comment.
"the bodicentric" -> "The bodycentric"
| x_spheroid = self._equatorial_radius * coslat * coslon | ||
| y_spheroid = self._equatorial_radius * coslat * sinlon | ||
| z_spheroid = self._equatorial_radius * (1 - self._flattening) * sinlat |
There was a problem hiding this comment.
Indeed! They are no longer needed as the calculation is done for r already.
| d = np.hypot(p, cart.z) | ||
| lat = np.arctan2(cart.z, p) | ||
| p_spheroid = cls._equatorial_radius * np.cos(lat) | ||
| z_spheroid = (cls._equatorial_radius * (1 - cls._flattening)) * np.sin(lat) |
There was a problem hiding this comment.
Total nitpick, but outer parentheses are not necessary.
| This is used internally for the standard Earth ellipsoids used in | ||
| `~astropy.coordinates.BaseGeodeticRepresentation` is used internally for the standard | ||
| Earth ellipsoids used in | ||
| `~astropy.coordinates.EarthLocation` |
There was a problem hiding this comment.
Maybe combine with last line while you are making the last changes.
There was a problem hiding this comment.
Sorry I'm not sure to understand this comment....
There was a problem hiding this comment.
Meant to combine the two lines to give```
Earth ellipsoids used in ~astropy.coordinates.EarthLocation
| recommended by the Cartographic Coordinates & Rotational Elements Working Group | ||
| (see for example its `2019 report <https://rdcu.be/b32WL>`_): the bodycentric latitude | ||
| and longitude are spherical latitude and longitude relative to the barycenter of the | ||
| body, the height is the distance from the spheroid surface on the line of sight. |
There was a problem hiding this comment.
"the height is the distance from the spheroid surface on the line of sight" -> "height is the distance from the surface (measured radially)."
|
|
||
| Creating Your Own Geodetic Representation | ||
| ----------------------------------------- | ||
| Creating Your Own Geodetic and Bodycentric Representation |
There was a problem hiding this comment.
Maybe make plural: "Representations"
|
@cmarmo - separately from this PR, my sense would be to try to follow this up with adding some of the I/O and WCS system things you have been mentioning rather than worry about west longitude and wrap angles! |
| Astropy also offers a `~astropy.coordinates.BaseGeodeticRepresentation` and | ||
| a `~astropy.coordinates.BaseBodycentricRepresentation` useful to | ||
| create specific representations on spheroidal bodies. | ||
| This is used internally for the standard Earth ellipsoids used in |
There was a problem hiding this comment.
I didn't mean to have this part about the internal use to be removed!
| Subclasses need to set attributes ``_equatorial_radius`` and ``_flattening`` | ||
| to quantities holding correct values (with units of length and dimensionless, | ||
| respectively). the bodicentric latitude and longitude are spherical latitude | ||
| respectively). the bodycentric latitude and longitude are spherical latitude |
There was a problem hiding this comment.
Still need to capitalize "The"
| The latitude is a value ranging from -90 to 90 degrees, the longitude from 0 to 360 | ||
| degrees. | ||
|
|
||
| `~astropy.coordinates.BaseGeodeticRepresentation` is used internally for the standard |
There was a problem hiding this comment.
Oops, sorry, you moved it here! This is better!
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This all looks great now. I think I'll just merge, the one uncapitalized "t" can be done at some other time!
|
Thanks, @cmarmo!! |
|
Thanks @mhvk for your patience! |
Description
This pull request adds the possibility to create geodetic representations described by [planetocentric latitudes]
(https://en.wikipedia.org/wiki/Latitude#Geodetic_and_geocentric_latitudes), west positive longitudes, longitudes spanning from 0 to 360 degrees.The default behavior is backward compatible.
Suggested in #11170 (comment).
Towards #11170.
This work is funded by the Europlanet 2024 Research Infrastructure (RI) Grant.