New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distribution fixes to work with EarthLocation #15304
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.
|
👋 Thank you for your draft pull request! Do you know that you can use |
3bb96d6
to
e7dbdf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mhvk, I think this is looking really good, definitely ready to go beyond "draft".
I have a few inline suggestions, but nothing major. I will admit I was not able to follow all of the numpy machinery, but I am satisfied enough that the tests are thorough and behave as expected.
I have some misgivings about the relatively dramatic-but-subtle API changes but I think they are already well-justified enough that's the benefits outweigh the costs.
|
||
if NUMPY_LT_2_0: | ||
from numpy.core.multiarray import normalize_axis_index | ||
from numpy.lib.function_base import _parse_gufunc_signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked and _parse_gufunc_signature
hasn't changed in 7 years, so this is probably not an issue, but... should we be worried it might change as a private function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should, and really the "true" gufunc parsing in the ufunc machinery should be exposed, which I hope to do one day, but for now I think it is fine to rely on our testing. Note that this is used in Masking
too, where tests are a little more rigorous.
# TODO: this covers the case where a user puts in a list or so, | ||
# but for those one could just explicitly do something like | ||
# _generated_subclasses[list] = NdarrayDistribution | ||
return NdarrayDistribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense to assume, but probably should document in the Disribution
main docstring that it will default to NdarrayDistribution if there's not a better choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point, done, now mention that NdarrayDistribution
is the general outcome for non-ndarray input.
astropy/uncertainty/core.py
Outdated
|
||
The distribution, with sampling along the *trailing* axis. If 1D, the sole | ||
dimension is used as the sampling axis (i.e., it is a scalar distribution). | ||
If an |ndarray| or subclass, the data will not be copied unless it is not | ||
possible to take a view (generally, only when the strides of the last axis | ||
are negative). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distribution, with sampling along the *trailing* axis. If 1D, the sole | |
dimension is used as the sampling axis (i.e., it is a scalar distribution). | |
If an |ndarray| or subclass, the data will not be copied unless it is not | |
possible to take a view (generally, only when the strides of the last axis | |
are negative). | |
The distribution, with sampling along the *trailing* axis. If 1D, the sole | |
dimension is used as the sampling axis (i.e., it is a scalar distribution). | |
If an |ndarray| or subclass, the data will not be copied unless it is not | |
possible to take a view (generally, only when the strides of the last axis | |
are negative). |
Not sure if this is strictly necessary (it was at one point but maybe it's changed?), but most of the other docstrings follow the indented-under-parameters convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, no, that is me trying to get line links in other, forgetting to remove a blank line, and then some pre-commit changing the indent. Thanks for noticing! Now corrected.
astropy/uncertainty/core.py
Outdated
# Sanity check. This should never happen! | ||
raise RuntimeError( | ||
f"found {base_cls=}, which does not have _DistributionRepr at " | ||
"__mro__[1]. Please raise an issue." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"__mro__[1]. Please raise an issue." | |
"__mro__[1]. Please raise an issue at " | |
"https://github.com/astropy/astropy/issues/new/choose." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I should not have been that lazy... Now corrected.
e7dbdf2
to
a48a000
Compare
@mhvk , you need to rebase to pick up matplotlib pin for RTD. Auto-merge is currently blocked. |
As part of this, define _get_distribution_cls in analogy to what is done in Masked._get_masked_cls. This is used both in the initializer and in .view().
Arguably, this is more logical, since we want the array to look like a regular array as much as possible. It is also required to get structured dtypes to work with Quantity.
This involves a second indirection in the actual dtype of the array, which now has a "samples" entry that itself consists of a n_samples "sample" entries. This is required to ensure we can get specific fields out of a distribution with a structured dtype.
With the new sample indirection, it becomes possible, by using stride trickery to ensure that, e.g., viewing a complex number as two floats will give the correct new shape.
…ution. Also update tests a bit to ensure correct exceptions are raised.
This involved ensuring __array_ufunc__ could deal with gufuncs properly, as well as making an initial __array_function__ which so far only supports np.empty_like.
This adds new information to the previous .feature one, and renames both it and the .api one to the correct PR number.
a48a000
to
470d8ae
Compare
# TODO: this part could be removed, with the try/except around the | ||
# assignment to struc["x"], ..., below. But this is a small API change | ||
# in that it will no longer possible to initialize with a unit-full x, | ||
# and unit-less y, z. Arguably, though, that would just solve a bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it annoyed, so I in fact have a follow-up PR ready to fix it! I decided not to apply that here since I felt it was good here to keep the changes to EarthLocation
as minimal as possible.
Now if only this didn't get messed up by the numpy 1.26 release, which broken our python 3.9 wheels build...
@@ -602,6 +602,7 @@ def _validate_angles(self, angles=None): | |||
# Ensure ndim>=1 so that comparison is done using the angle dtype. | |||
# Otherwise, e.g., np.array(np.pi/2, 'f4') > np.pi/2 will yield True. | |||
# (This feels like a bug -- see https://github.com/numpy/numpy/issues/23247) | |||
# TODO: address this again when/if numpy 2.0 exists - see above issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a follow up issue for this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from @eteq's commit, so I don't take responsibility, but I think we're OK: our current code works fine, we can adjust whenever someone sees the TODO
; a sort-of implicit issue, I guess!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of text for a change log. You sure some of this doesn't belong in user doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about user doc? Seems like an undocumented new feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current documentation does not go even close to the detailed level here... In a way, all this is an implementation detail (which is why I felt I could change it in the first place), so this API change notice is really addressed to people who actually use Distribution
in gory detail (if any exist).
Note that I do hope to update the documentation once I can use Distribution
in SkyCoord
(not too far off, I'm hoping).
This pull request fixes
Distribution
so that it can work withEarthLocation
. It does not yet check all the methods, but initialization via both geocentric and geodetic coordinates. Builds on #15296 (which in turn builds on #15295). so review by commit may be easiest.Supercedes #14653 (and contains the commits from that; hence preferably no squash merge)