Distribution fixes to work with EarthLocation by mhvk · Pull Request #15304 · astropy/astropy

mhvk · 2023-09-11T17:21:33Z

This pull request fixes Distribution so that it can work with EarthLocation. 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)

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2023-09-11T17:22:16Z

github-actions · 2023-09-11T17:22:22Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

eteq

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.

eteq · 2023-09-15T18:36:59Z

+
+if NUMPY_LT_2_0:
+    from numpy.core.multiarray import normalize_axis_index
+    from numpy.lib.function_base import _parse_gufunc_signature


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?

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.

eteq · 2023-09-15T18:41:47Z

+                # 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


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?

Yes, good point, done, now mention that NdarrayDistribution is the general outcome for non-ndarray input.

eteq · 2023-09-15T18:48:26Z

+
+    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).


Suggested change

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

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.

eteq · 2023-09-15T18:49:21Z

+                # Sanity check. This should never happen!
+                raise RuntimeError(
+                    f"found {base_cls=}, which does not have _DistributionRepr at "
+                    "__mro__[1]. Please raise an issue."


Suggested change

"__mro__[1]. Please raise an issue."

"__mro__[1]. Please raise an issue at "

"https://github.com/astropy/astropy/issues/new/choose."

Yes, I should not have been that lazy... Now corrected.

mhvk · 2023-09-16T14:05:53Z

@eteq - Thanks so much for the quick review. As noted inline, I made the changes and set this to auto-merge since there is other stuff hanging on it, and I really want to clean my mess of branches up a bit! Plus, then I can rebase my old #14423 on this and also get Representation working!

pllim · 2023-09-16T14:50:48Z

@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.

mhvk · 2023-09-17T09:24:43Z

@pllim - thanks! Rebased with the matplotlib 3.8 fixes in, so now RTD does not fail anymore, but we got a new one (also unrelated, grrrr), cp39-manylinux wheel building is failing -- see #15333. Sorry to be the bearer of this further bad news!

nstarman · 2023-09-17T13:06:11Z

+        # 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.


I agree it's a bug.

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...

pllim · 2023-09-18T12:55:14Z

        # 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


Is there a follow up issue for this TODO?

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!

pllim · 2023-09-18T12:56:32Z

This is a lot of text for a change log. You sure some of this doesn't belong in user doc?

pllim · 2023-09-18T12:57:02Z

What about user doc? Seems like an undocumented new feature?

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).

mhvk added coordinates Enhancement uncertainty labels Sep 11, 2023

mhvk added this to the v6.0 milestone Sep 11, 2023

mhvk requested a review from eteq September 11, 2023 17:21

mhvk mentioned this pull request Sep 11, 2023

Implement late review suggestions on #14421 #14653

Closed

mhvk force-pushed the distribution-earthlocation branch from 3bb96d6 to e7dbdf2 Compare September 11, 2023 17:56

eteq approved these changes Sep 15, 2023

View reviewed changes

mhvk force-pushed the distribution-earthlocation branch from e7dbdf2 to a48a000 Compare September 16, 2023 14:02

mhvk added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Sep 16, 2023

mhvk marked this pull request as ready for review September 16, 2023 14:03

This was referenced Sep 16, 2023

Allow structured data types in Distribution #15296

Closed

Refactor Distribution a little (in preparation for structured ones) #15295

Closed

mhvk enabled auto-merge September 16, 2023 14:07

mhvk and others added 11 commits September 17, 2023 10:59

Test that Distribution can deal with advanced indices

f4deb82

Get distribution class in view rather than reinitialize.

45e4447

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().

Let the Distribution dtype be that of a single sample.

9aaadd9

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.

Allow .view that would work for regular arrays as well.

3f73cb0

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.

Changelog entry and docstring update

87fcf14

minor comment clarification

0170175

add earthlocations tests although they xfail

f47dfba

Small fixes to EarthLocation required for making it work with Distrib…

2b32e2e

…ution. Also update tests a bit to ensure correct exceptions are raised.

Ensure EarthLocationDistributions are possible.

fb3701b

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.

Correct hangelog entries.

470d8ae

This adds new information to the previous .feature one, and renames both it and the .api one to the correct PR number.

mhvk force-pushed the distribution-earthlocation branch from a48a000 to 470d8ae Compare September 17, 2023 09:00

nstarman reviewed Sep 17, 2023

View reviewed changes

pllim reviewed Sep 18, 2023

View reviewed changes

pllim disabled auto-merge September 18, 2023 13:23

pllim merged commit 763ee0c into astropy:main Sep 18, 2023

mhvk deleted the distribution-earthlocation branch September 18, 2023 14:07

mhvk mentioned this pull request Sep 24, 2023

Structured Quantity Distribution #12909

Closed

This was referenced Apr 30, 2026

BUG: crash (segfault) bisected to gh-29270 numpy/numpy#31363

Closed

TST: devdeps segfault #19621

Closed

	"__mro__[1]. Please raise an issue."
	"__mro__[1]. Please raise an issue at "
	"https://github.com/astropy/astropy/issues/new/choose."

Conversation

mhvk commented Sep 11, 2023

Uh oh!

github-actions Bot commented Sep 11, 2023

Uh oh!

github-actions Bot commented Sep 11, 2023

Uh oh!

eteq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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 commented Sep 16, 2023

Uh oh!

pllim commented Sep 16, 2023

Uh oh!

mhvk commented Sep 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eteq left a comment •

edited

Loading