Add support for gc2gde and gd2gce erfa functions in units. by cmarmo · Pull Request #14729 · astropy/astropy

cmarmo · 2023-05-01T21:25:51Z

Description

This pull request adds support for gc2gde and gd2gce erfa functions in units.
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-01T21:26:25Z

cmarmo · 2023-05-02T00:18:44Z

Note that I can't reproduce the coverage failure locally ... any suggestion is welcome! Thanks!

pllim · 2023-05-02T04:22:22Z

The coverage failure was because coverage report upload fails. Happens once in a while.

Still needs a change log. Thanks!

mhvk

Thanks! A few comments on the implementation...

mhvk · 2023-05-02T09:46:11Z

        )


+def helper_gd2gce(f, unit1, dimensionless_unscaled, unit2, unit3, unit4):


Here it should just enumerate all units - including that for flattening.

Thanks and sorry, now I better understand what dimensionless_unscaled is...

mhvk · 2023-05-02T09:47:03Z

+    try:
+        return [
+            get_converter(unit1, m),
+            None,


and then here add get_converter(unit2, dimensionless_unscaled)

mhvk · 2023-05-02T09:48:12Z

+        ], (m, None)
+    except Exception as exc:
+        raise UnitTypeError(
+            f"Can only apply '{f.__name__}' function to lon, lat "


Looking through the other functions in this file, I see that normally no try/except was done. I think the only reason I did it for the gc2gd and gd2gc ones is that those functions are special in that they also take an integer, i.e., not a Quantity. So, I think it is actually fine to remove the try/except here and just let the get_converter functions give out their own error messages as needed. (As done for helper_s2pv, e.g.)

mhvk · 2023-05-02T10:04:26Z

        )


+def helper_gc2gde(f, unit1, dimensionless_unscaled, unit2):


Like below, enumerate all units, and have get_converter(unit2, dimensionless_unscaled) below.

By the way, feel free to give the units more meaningful names - I know I didn't do it for many functions but I changed later (see, e.g., helper_s2pv).

mhvk

Sorry, a few more comments. Some noting that really the existing code can use a bit of improvement... Sigh...

mhvk · 2023-05-02T21:27:10Z

+def helper_gc2gde(f, unit_r, unit_flat, unit_xyz):
+    from astropy.units.si import m, radian
+
+    if unit_r is None:


I don't think this is necessary - just let get_converter handle this!

I realize the error messages may not be ideal, but I think it may be better in that case to try to improve get_converter -- but in a different PR!

mhvk · 2023-05-02T21:27:33Z

+def helper_gd2gce(f, unit_r, unit_flat, unit_long, unit_lat, unit_h):
+    from astropy.units.si import m, radian
+
+    if unit_r is None:


Same, just keep it simple!

mhvk · 2023-05-02T21:28:12Z

+
+    if unit_r is None:
+        raise UnitTypeError("Equatorial radius should have length units.")
+    unit_flat = unit_r / unit_r


Why is this necessary? You are overriding the input unit (which might be, say, u.percent, which could be converted to dimensionless_unscaled)

ok, understood: this was the reason of the error on unit_r being managed separately.
I'm going to remove it then the check on unit_r will go too.

mhvk · 2023-05-02T21:33:08Z

+                self.lat,
+                self.height,
+            )
+        msg = "'NoneType' object has no attribute '_get_converter'"


That's actually a pretty terrible error message! I think I definitely see a point now of trying to do better - but let's do so in a different PR!

cmarmo · 2023-05-02T21:56:17Z

@mhvk I just realized that the erfa.eform function (which I need to retrieve the ellipsoid parameters from the standard string, when addressing the item .2.) does not have its own helper.
I'm going to add it here too if this is ok for you.

mhvk · 2023-05-03T08:33:00Z

+def helper_eform(f, nounit):
+    if nounit is not None:
+        raise UnitTypeError("Ellipsoid cannot be a quantity.")
+    return [None], (None, None)


Good idea to add coverage for eform too! But this doesn't seem right: the equatorial radius and flattening do have units: (m, dimensionless_unscaled) (and from astropy.units.si import m above)

That was my first try... I couldn't make it work.
No matter how I changed the output dimensions, eform always gives me a-dimensional outputs...
I have pushed the "dimensional" version for you to check... I'm probably missing something.

Ah, it actually makes sense - I should have thought of this! The overrides only work if one puts in a Quantityas input (or output). Without that, the ufunc has no way to know a Quantity is expected and therefore just acts as if it regular ndarray input. We can actually not change this in astropy in any simple way.

In principle, the helper function will only be called if you put in a Quantity to hold the output. This is not in itself bad, but also not super useful. Perhaps the simplest right now is to just remove this again from the PR, and then in the next PR just work with the raw version, i.e., call it and then turn the output into Quantity there.

Perhaps the simplest right now is to just remove this again from the PR, and then in the next PR just work with the raw version, i.e., call it and then turn the output into Quantity there.

Thanks for your explanation! Makes sense... :)
Just to be sure: you mean I should go back to 34b642f or completely remove eform from the helpers? Thanks!

I would remove it altogether for now - I think it needs more thought, but is not essential for BodyLocation, but rather an implementation detail.

cmarmo · 2023-05-04T10:41:36Z

I have removed eform from this pull request, synchronized with main and squashed the commits.
Thanks for your patience.

mhvk

No, really, thanks for your patience! I have some final nitpicks, but am approving already, since things look good!

mhvk · 2023-05-04T11:05:49Z

+
+        msg = "'NoneType' object has no attribute '_get_converter'"
+        xyz = np.array([self.x_value, self.y_value, self.z_value]) << self.length_unit
+        status = 0


No need to set status = 0 beforehand, since you're not passing it in. Indeed, since we're just looking for the error message, you can just call the ufunc without assigning to any outputs.

mhvk · 2023-05-04T11:07:31Z

+        """Test unit errors when dimensionless parameters are used"""
+
+        msg = "'NoneType' object has no attribute '_get_converter'"
+        xyz = np.array([self.x_value, self.y_value, self.z_value]) << self.length_unit


Real nitpick, but might as well do xyz = np.stack([self.x, self.y, self.z]).
Indeed, since you use it multiple times, might as well define it in setup_class

mhvk · 2023-05-04T11:08:39Z

+                self.lat,
+                self.height,
+            )
+        xyz = np.array([self.x_value, self.y_value, self.z_value])


Continuing with nitpicks, how about removing this and instead do xyz.value in the call below?

cmarmo · 2023-05-04T18:13:08Z

nitpicks addressed... :)

mhvk

OK, all good now - let get it in! Thanks!!

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

Wonderful! Thanks! 🤗 🥳

pllim · 2023-05-06T16:05:41Z

From the sidelines, I really appreciate all these non-trivial fixes both in FITS and coordinates, @cmarmo ! Looking forward to more. 👏

pllim · 2023-05-06T16:07:23Z

Europlanet 2024 Research Infrastructure (RI) Grant

This is amazing! I am so glad to see more grants agreeing to invest dev time into the Project. cc @astropy/coordinators

cmarmo · 2023-05-06T20:18:16Z

This is amazing! I am so glad to see more grants agreeing to invest dev time into the Project

This was possible thanks to @Erard and the involvement of the Observatoire de Paris in the Europlanet project. 🙏

hamogu · 2023-06-12T16:29:10Z

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

It's great that funded projects contribute to astropy. That's exactly how Open source is supposed to work and we welcome your contributions. I'm just commenting here to let you know that you of course you are also welcome to connect on slack, on the astropy-dev mailing list or by email to the Coordination Committee if you ever need something "official looking" for e.g. a grant proposal.

cmarmo · 2023-06-14T09:13:46Z

Thanks @hamogu!
Apparently I cannot create an account on slack as I don´t have the right domain... :)
I may need an invitation? Thanks!

hamogu · 2023-06-14T09:15:30Z

Maybe the slack link expired. I'll check with the slack admins and get back to you.

hamogu · 2023-06-14T09:18:17Z

Can you try: http://joinslack.astropy.org/ ?

cmarmo · 2023-06-14T09:31:29Z

That worked! Thanks a lot!

Maybe the slack link expired.

...my bad for being off-line for some time...

hamogu · 2023-06-14T09:55:57Z

I think I just linked the wrong link in #14729 (comment)
Since I'm signed up already, I did not notice the difference between sign-in and sign-up. Sorry ;-)
But I'm glad it worked!

github-actions Bot added coordinates units labels May 1, 2023

cmarmo mentioned this pull request May 1, 2023

Refactor EarthLocation as instance of BodyLocation. Add flattening and equatorial radius as parameters. #14714

Closed

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

pllim requested a review from mhvk May 2, 2023 04:21

mhvk reviewed May 2, 2023

View reviewed changes

cmarmo changed the title ~~Add support for gc2gde and gd2gce erfa functions in units.~~ Add support for eform, gc2gde and gd2gce erfa functions in units. May 3, 2023

mhvk reviewed May 3, 2023

View reviewed changes

Add helpers for gc2gde and gd2gce. Rename testfile. Add tests.

ed559f3

cmarmo force-pushed the erfa-ufunc-units branch from bf8fb79 to ed559f3 Compare May 4, 2023 10:39

cmarmo changed the title ~~Add support for eform, gc2gde and gd2gce erfa functions in units.~~ Add support for gc2gde and gd2gce erfa functions in units. May 4, 2023

mhvk approved these changes May 4, 2023

View reviewed changes

Clean tests.

9ad2f1f

mhvk approved these changes May 5, 2023

View reviewed changes

mhvk merged commit 96df795 into astropy:main May 5, 2023

cmarmo deleted the erfa-ufunc-units branch May 5, 2023 18:32

		)


		def helper_gd2gce(f, unit1, dimensionless_unscaled, unit2, unit3, unit4):

		)


		def helper_gc2gde(f, unit1, dimensionless_unscaled, unit2):

Conversation

cmarmo commented May 1, 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 1, 2023 • edited by mhvk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmarmo commented May 2, 2023

Uh oh!

pllim commented May 2, 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!

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!

cmarmo commented May 2, 2023

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 4, 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!

cmarmo commented May 4, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

cmarmo commented May 5, 2023

Uh oh!

pllim commented May 6, 2023

Uh oh!

pllim commented May 6, 2023

Uh oh!

cmarmo commented May 6, 2023

Uh oh!

hamogu commented Jun 12, 2023

Uh oh!

cmarmo commented Jun 14, 2023

Uh oh!

hamogu commented Jun 14, 2023

Uh oh!

cmarmo commented May 1, 2023 •

edited

Loading

github-actions Bot commented May 1, 2023 •

edited by mhvk

Loading