Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. by cmarmo · Pull Request #14763 · astropy/astropy

cmarmo · 2023-05-05T18:55:41Z

Description

This pull request adds support for equatorial_radius and flattening in BaseGeodeticRepresentation.
~~A new class StandardGeodeticRepresentation is added inheriting from BaseGeodeticRepresentation, retrieving~~ ~~spheroid attributes from a standard code.~~
All geodetic representations classes are factorized into geodetic.py.

Tests are added.

Follows the suggestion in #14714 (review).

Towards #11170.

Thanks for considering it.

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

github-actions · 2023-05-05T18:56:26Z

cmarmo · 2023-05-06T20:24:37Z

I am able to reproduce the test failure locally with

python -m tox -e py311-test-double

I am still trying to understand why ... 🤔

mhvk

Thanks, this looks great, and hence most of my comments (and perhaps confusion) are about names... I haven't quite figured out the best way forward, but think I better just submit my present review since you may have better ideas.

p.s. Review is far easier with things split out - thanks very much for humouring me!

mhvk · 2023-05-07T20:44:04Z

Having just added the "whatsnew-needed" label, I think what we also have to do is to add some documentation... I think a short additional section at the end of https://docs.astropy.org/en/latest/coordinates/representations.html#creating-your-own-representations should be good enough for now.

cmarmo · 2023-05-08T03:31:09Z

@mhvk thanks for your review!
Before addressing your comments, I would like to clarify the issue about file/module and class names.

My understanding is that the representation module-to-be contains classes representing different ways of describing the coordinates. The current BaseGeodeticClass is defined by two angles and a length, where the latitude is defined by the angle between the x-axis and the vertical to the surface at a specific point of the spheroid.
Be the spheroid defined by a standard code or flattening and equatorial radius, does not change the essence of the representation. This is not the same as eg the spherical representation and the unit sphere representation: in that case the needed coordinates really change (r is not needed in the second).
That said, I would like to ask you to reconsider the creation of a new class (the one I called StandardGeodeticRepresentation): this will automatically solve some of your comments (Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. #14763 (comment) and Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. #14763 (comment)) also forcing me to add checks about which attribute is defined like in Refactor EarthLocation as instance of BodyLocation. Add flattening and equatorial radius as parameters. #14714 (and answering Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. #14763 (comment)).
I understand the code itself looks cleaner here, but in my opinion an additional level of abstraction is unnecessary as it does not correspond to a different representation, and perhaps we are overcomplicating things for users.
I believe we can safely use the name geodetic.py for the file containing the geodetic representation (even when generalized to other planets) as this is already done by the NAIF SPICE routines: for example the routines corresponding to gc2gde and gd2gc are recgeo and georec.
A useful summary is available here: geodetic is the term used by NAIF SPICE "Applicable to bodies with equal equatorial radii."
We may be more precise using planetodetic but then we might want to change the names of all the functions defined in EarthLocation too and I'm wondering if this is worth it.

Let me know how do you feel about those suggestions.
Thanks!

mhvk · 2023-05-08T13:45:11Z

@cmarmo - when I did my review yesterday I also wondered if it was actually necessary to have two subclasses, and whether, instead, we should not just return closer to what you had, where BaseGeodeticRepresentation.__init_subclass__ just checks for _ellipsoid, and, if found, then sets _equatorial_radius and _flattening based on it.

Also, good to know that the consensus in the community is to just call it geodetic even for other planetary bodies. If so, let's go for it.

Anyway, it sounds like we have a plan!

p.s. It doesn't have to be now, but if you think it is better, I'd be fine with moving the explicit representations from earth.py to geodetic.py, i.e., just leaving EarthLocation only in earth.py. Once representations becomes its own module, then geodetic can move there, which would be quite logical.

cmarmo · 2023-05-08T20:03:59Z

Anyway, it sounds like we have a plan!

Wonderful! :)
I have addressed some of your comments ... if you don't mind I'm going to resolve some of them: I realized I'm starting to miss inline comments and I need a way to check those I have addressed.... getting old... ;)

mhvk

Now definitely moving towards minimal comments and nitpicks - I think overall this is looking very good!

But obviously need to figure out why the tests are failing. My guess is that it is the registration of the test classes, and I give a suggestion in-line on how to fix this...

To do is a what's-new entry and documentation!

mhvk

Great! Code is now all OK - the diff makes me feel this is exactly right, with just a clean chunk moved from earth.py to the new geodetic.py, and logical additions.

Most of the remaining comments are just textual, on the added documentation.

One thought (not for this PR!!): it might make sense to allow one to input a BaseGeodeticRepresentation subclass in EarthLocation.to_geodetic (maybe as the ellipsoid argument), so people can use their custom classes in there.

mhvk · 2023-05-09T08:46:33Z

 * `~astropy.coordinates.CylindricalRepresentation`:
  cylindrical polar coordinates, represented by a cylindrical radius
  (``rho``), azimuthal angle (``phi``), and height (``z``).
+* `~astropy.coordinates.BaseGeodeticRepresentation`:


This class is different in that it is not useful in itself but has to be subclassed. How about here describing WGS84GeodeticRepresentation (which is useful in itself), and then at the end noting the two other pre-defined ones, and giving a link to your new section about how to create one's own?

This class is different in that it is not useful in itself but has to be subclassed.

I understand this point.
But then WGS84GeodeticRepresentation is a specific case, which is not true for the other items in the list.
What about removing the geodetic description from the list and adding a paragraph like:
"Astropy also offers a BaseGeodeticRepresentation useful to create specific representations on spheroidal bodies... blah...blah..."

eerovaher

I have some comments regarding the tests.

eerovaher · 2023-05-09T18:29:59Z

+def test_custombasegeodetic_error():
+    # Test incomplete initialization.


I'd like the test names to be descriptive enough that such comments would not be necessary. pytest shows the names of failing tests and they can be also displayed by running pytest with increased verbosity, but reading the comment requires looking up the source code for the test.

eerovaher · 2023-05-09T18:33:43Z

+    msg = "module 'erfa' has no attribute 'foo'"
+    with pytest.raises(AttributeError, match=msg):

+        class CustomGeodeticEllipsoidError(BaseGeodeticRepresentation):


If I read the code I can figure out why an error is implemented as a subclass of BaseGeodeticRepresentation, but I'd rather not spend the effort.

Suggested change

class CustomGeodeticEllipsoidError(BaseGeodeticRepresentation):

class InvalidCustomGeodeticEllipsoid(BaseGeodeticRepresentation):

This also applies to the other class in this function.

eerovaher · 2023-05-09T18:41:30Z

+    s1g = CustomGeodetic(
+        lon=[0.8, 1.3] * u.radian,
+        lat=[0.3, 0.98] * u.radian,
+        height=[100.0, 367.0] * u.m,
+    )
+
+    s2g = CartesianRepresentation.from_representation(s1g)
+
+    s3g = CustomGeodetic.from_representation(s2g)


I don't like having variables that are used exactly once because I don't know that they'll be discarded almost immediately when I'm reading the code, so I try to memorize what they are only to find out very quickly that I can forget about them entirely.

Suggested change

s1g = CustomGeodetic(

lon=[0.8, 1.3] * u.radian,

lat=[0.3, 0.98] * u.radian,

height=[100.0, 367.0] * u.m,

)

s2g = CartesianRepresentation.from_representation(s1g)

s3g = CustomGeodetic.from_representation(s2g)

initial = CustomGeodetic(

lon=[0.8, 1.3] * u.radian,

lat=[0.3, 0.98] * u.radian,

height=[100.0, 367.0] * u.m,

)

roundtripped = CustomGeodetic.from_representation(

CartesianRepresentation.from_representation(initial)

)

eerovaher · 2023-05-09T18:46:49Z

+    assert "customgeodeticerror" not in REPRESENTATION_CLASSES
+
+
+def test_custombasegeodetic_roundtrip():


This test should be parametrized. If one of the asserts fails then the following asserts in the test function are not executed, but in the case of a parametrized test one of the parametrizations failing does not prevent other parametrizations of the same test from running, so a parametrized test suite is more granular and informative with no or very little extra effort.

eerovaher · 2023-05-09T18:48:05Z

+
+    s3g = CustomGeodetic.from_representation(s2g)
+
+    assert quantity_allclose(s1g.lon, s3g.lon)


It is better to use assert_quantity_allclose() because it is more informative if the assertion fails.

cmarmo · 2023-05-09T20:54:07Z

@eerovaher , Thanks for your comments, I believe I have addressed all your concern?

@mhvk something is wrong with the conversion of _flattening... I am confused.
I had to manually force the conversion between a float and a dimensionless_unscaled, ~~also the docstring is failing with~~ ~~a % since I have defined the flattening as u.percent~~

EDIT: removing a debugging print helped a lot... 😅

mhvk · 2023-05-10T13:52:48Z

@cmarmo - the reason that _flattening didn't work is that we didn't quite do the ufunc right for that - my mistake to not notice that, really. I made a change locally, and also some small further changes to the tests (to split them up further as requested by @eerovaher and minimize the difference with what was there), but I cannot push to your branch. Could you allow me to? (Need to "Enable edits by maintainers".). If that's tricky, the rebased/squashed branch including a commit for the helpers is at https://github.com/mhvk/astropy/tree/representations-geodetic - you could get that and push to your branch yourself.

mhvk · 2023-05-10T17:24:48Z

p.s. Just updated my branch with an additional commit that moves geodetic inside astropy.coordinates.representations (which was made its own submodule in #14792). It also corrected your commit to put back in that infamous noqa: F401 which also for me had been automatically removed.

cmarmo · 2023-05-10T18:23:48Z

I cannot check the "allow edits from maintainers" because of https://github.com/orgs/community/discussions/5634.
I'm going to make some manipulations locally ... back soon.

…sentation. Add tests and documentation.

cmarmo · 2023-05-10T20:00:06Z

I have pushed a new version of the documentation implementing my suggestion in answering this comment. Let me know if it is acceptable.

mhvk

Sorry for the trouble, I hadn't realized group accounts were different!
And also sorry that my PR missed some things!

But it now all looks great! I have one possible suggestion for the documentation, but am also happy to merge as is, so will leave that up to you and approve now.

mhvk · 2023-05-10T22:20:37Z


+
+Astropy also offers a `~astropy.coordinates.BaseGeodeticRepresentation` useful to
+create specific representations on spheroidal bodies (see


I think this paragraph is very clear! Should we mention the existing earth-based ones? E.g.,

Astropy also offers a `~astropy.coordinates.BaseGeodeticRepresentation` useful to create specific `repreentations` on spheroidal bodies. This is used internally for the standard Earth ellipsoids used in `~astropy.coordinates.EarthLocation` (`~astropy.coordinates.WGS84GeodeticRepresentation`, `~astropy.coordinates.WGS72GeodeticRepresentation`, and `~astropy.coordinates.GRS80GeodeticRepresentation`), but can also be customized; see :ref:`astropy-coordinates-create-geodetic`. All these are coordinates on ...

I like it! I'm adding it right now.

mhvk · 2023-05-11T09:01:53Z

The tests seem to be stuck, so I'm going to close & reopen. Hopefully, that will resolve it. Otherwise, everything is OK, so I'm going to try our new "auto-merge" feature

cmarmo · 2023-05-11T09:37:23Z

Thanks! 🥳

github-actions Bot added the coordinates label May 5, 2023

cmarmo force-pushed the geodetic-representation-pr branch 2 times, most recently from 5eeecd0 to 923ce18 Compare May 5, 2023 22:27

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

pllim requested a review from mhvk May 5, 2023 23:00

mhvk reviewed May 7, 2023

View reviewed changes

mhvk added Enhancement whatsnew-needed labels May 7, 2023

mhvk mentioned this pull request May 8, 2023

Refactor representations as a module #14792

Merged

cmarmo force-pushed the geodetic-representation-pr branch from 923ce18 to 042d51f Compare May 8, 2023 20:00

mhvk reviewed May 8, 2023

View reviewed changes

cmarmo force-pushed the geodetic-representation-pr branch from 7a62c19 to 6dce627 Compare May 9, 2023 01:08

cmarmo changed the title ~~Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. Add StandardGeodeticRepresentation.~~ Add support for equatorial_radius and flattening in BaseGeodeticRepresentation. May 9, 2023

cmarmo force-pushed the geodetic-representation-pr branch from 6dce627 to 5007cf7 Compare May 9, 2023 01:14

cmarmo commented May 9, 2023

View reviewed changes

Comment thread docs/coordinates/representations.rst Outdated

mhvk reviewed May 9, 2023

View reviewed changes

eerovaher reviewed May 9, 2023

View reviewed changes

cmarmo force-pushed the geodetic-representation-pr branch from a5e35a3 to a10c965 Compare May 10, 2023 19:00

cmarmo and others added 2 commits May 10, 2023 09:12

Add support for equatorial_radius and flattening in BaseGeodeticRepre…

87b35a1

…sentation. Add tests and documentation.

Ensure we can use floats for flattening, since they are dimensionless

c06d3b8

cmarmo force-pushed the geodetic-representation-pr branch from 5eb5e6d to a30f021 Compare May 10, 2023 19:13

mhvk approved these changes May 10, 2023

View reviewed changes

mhvk and others added 2 commits May 10, 2023 12:29

Move geodetic inside new representation module

9bbf613

Improve documentation.

e6c0a86

cmarmo force-pushed the geodetic-representation-pr branch from c08a964 to e6c0a86 Compare May 10, 2023 22:30

cmarmo mentioned this pull request May 11, 2023

Planetary WCS implementation in Astropy #11170

Closed

mhvk enabled auto-merge May 11, 2023 09:01

mhvk closed this May 11, 2023

auto-merge was automatically disabled May 11, 2023 09:01
Pull request was closed

mhvk reopened this May 11, 2023

mhvk enabled auto-merge May 11, 2023 09:02

mhvk merged commit 80175d7 into astropy:main May 11, 2023

cmarmo deleted the geodetic-representation-pr branch May 11, 2023 09:37

braingram mentioned this pull request May 15, 2023

[pre-commit.ci] pre-commit autoupdate astropy/asdf-astropy#178

Merged

braingram mentioned this pull request May 23, 2023

Update RepresentationConverter for new class paths in astropy dev astropy/asdf-astropy#181

Merged

		def test_custombasegeodetic_error():
		# Test incomplete initialization.

	class CustomGeodeticEllipsoidError(BaseGeodeticRepresentation):
	class InvalidCustomGeodeticEllipsoid(BaseGeodeticRepresentation):

		assert "customgeodeticerror" not in REPRESENTATION_CLASSES


		def test_custombasegeodetic_roundtrip():


		s3g = CustomGeodetic.from_representation(s2g)

		assert quantity_allclose(s1g.lon, s3g.lon)



		Astropy also offers a `~astropy.coordinates.BaseGeodeticRepresentation` useful to
		create specific representations on spheroidal bodies (see

Conversation

cmarmo commented May 5, 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 5, 2023

Uh oh!

cmarmo commented May 6, 2023

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!

Uh oh!

Uh oh!

mhvk commented May 7, 2023

Uh oh!

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

Uh oh!

Uh oh!

mhvk commented May 8, 2023

Uh oh!

cmarmo commented May 8, 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!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerovaher 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!

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

Uh oh!

Uh oh!

mhvk commented May 10, 2023

Uh oh!

mhvk commented May 10, 2023

Uh oh!

cmarmo commented May 5, 2023 •

edited

Loading

cmarmo commented May 8, 2023 •

edited

Loading

cmarmo commented May 8, 2023 •

edited

Loading

cmarmo commented May 9, 2023 •

edited

Loading