Add support for gc2gde and gd2gce erfa functions in units. #14729
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.
|
|
Note that I can't reproduce the coverage failure locally ... any suggestion is welcome! Thanks! |
|
The coverage failure was because coverage report upload fails. Happens once in a while. Still needs a change log. Thanks! |
| @@ -117,6 +134,24 @@ def helper_gd2gc(f, nounit, unit1, unit2, unit3): | |||
| ) | |||
|
|
|||
|
|
|||
| def helper_gd2gce(f, unit1, dimensionless_unscaled, unit2, unit3, unit4): | |||
There was a problem hiding this comment.
Here it should just enumerate all units - including that for flattening.
There was a problem hiding this comment.
Thanks and sorry, now I better understand what dimensionless_unscaled is...
| try: | ||
| return [ | ||
| get_converter(unit1, m), | ||
| None, |
There was a problem hiding this comment.
and then here add get_converter(unit2, dimensionless_unscaled)
| ], (m, None) | ||
| except Exception as exc: | ||
| raise UnitTypeError( | ||
| f"Can only apply '{f.__name__}' function to lon, lat " |
There was a problem hiding this comment.
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.)
| @@ -98,6 +99,22 @@ def helper_gc2gd(f, nounit, unit1): | |||
| ) | |||
|
|
|||
|
|
|||
| def helper_gc2gde(f, unit1, dimensionless_unscaled, unit2): | |||
There was a problem hiding this comment.
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).
| def helper_gc2gde(f, unit_r, unit_flat, unit_xyz): | ||
| from astropy.units.si import m, radian | ||
|
|
||
| if unit_r is None: |
There was a problem hiding this comment.
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!
| 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: |
|
|
||
| if unit_r is None: | ||
| raise UnitTypeError("Equatorial radius should have length units.") | ||
| unit_flat = unit_r / unit_r |
There was a problem hiding this comment.
Why is this necessary? You are overriding the input unit (which might be, say, u.percent, which could be converted to dimensionless_unscaled)
There was a problem hiding this comment.
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.
| self.lat, | ||
| self.height, | ||
| ) | ||
| msg = "'NoneType' object has no attribute '_get_converter'" |
There was a problem hiding this comment.
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
changed the title
| def helper_eform(f, nounit): | ||
| if nounit is not None: | ||
| raise UnitTypeError("Ellipsoid cannot be a quantity.") | ||
| return [None], (None, None) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
changed the title
|
I have removed |
|
|
||
| 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 |
There was a problem hiding this comment.
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.
| """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 |
There was a problem hiding this comment.
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
| self.lat, | ||
| self.height, | ||
| ) | ||
| xyz = np.array([self.x_value, self.y_value, self.z_value]) |
There was a problem hiding this comment.
Continuing with nitpicks, how about removing this and instead do xyz.value in the call below?
|
nitpicks addressed... :) |
|
Wonderful! Thanks! 🤗 🥳 |
|
From the sidelines, I really appreciate all these non-trivial fixes both in FITS and coordinates, @cmarmo ! Looking forward to more. 👏 |
This is amazing! I am so glad to see more grants agreeing to invest dev time into the Project. cc @astropy/coordinators |
This was possible thanks to @Erard and the involvement of the Observatoire de Paris in the Europlanet project. 🙏 |
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. |
|
Thanks @hamogu! |
|
Maybe the slack link expired. I'll check with the slack admins and get back to you. |
|
Can you try: http://joinslack.astropy.org/ ? |
|
That worked! Thanks a lot!
...my bad for being off-line for some time... |
|
I think I just linked the wrong link in #14729 (comment) |
Description
This pull request adds support for
gc2gdeandgd2gceerfa functions inunits.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.