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
Ensure Distribution can be used in Latitude and Longitude #14421
Ensure Distribution can be used in Latitude and Longitude #14421
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.
|
e2ce4fe
to
5631ab9
Compare
It would certainly be good if >>> from astropy import units as u
>>> from astropy.coordinates import Longitude
>>> from astropy.uncertainty import Distribution
>>> d = Distribution([359, 361] * u.deg)
>>> d.pdf_std()
<Quantity 1. deg>
>>> Longitude(d).pdf_std()
<Longitude 179. deg> |
Hmm, yes, that is unavoidable right now... Of course, the problem is that a standard deviation of a Maybe we should have an option to use circular for them? And default to those for p.s. Not sure we have to solve the above for this PR... @eerovaher - what do you think, shall I just open another issue? |
That seems the right solution. |
I agree that
I'd rather have this sorted out in this PR, especially if the implementation is simple. The ability to create a |
I was thinking of a @eerovaher - I don't quite agree that |
Or, rephrasing, I'd rather have PRs be small and to the point. This one fixes real bugs -- of not being able to compare with In contract, having circular |
Hmm, I thought we could simply use routines in |
https://insidebigdata.com/2021/02/12/circular-statistics-in-python-an-intuitive-intro/ |
Nice! I opened a feature request for more circular statistics, as well as discussion on how to implement it for |
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.
Let's proceed in small steps then and limit the scope of the pull request to what has already been implemented.
I do have a question about test setup. What is the advantage of implementing classes with a setup_class()
method over using pytest
fixtures or perhaps global constants? There is an obvious downside to implementing test classes - they use up a level of indentation.
``dist[dist<0] *= -1`` work. This also ensures that the ``Angle`` subclasses | ||
``Longitude`` and ``Latitude`` can be initialized with a ``Distribution``. |
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'm not sure we want to advertise initializing Angle
subclasses with Distribution
quite yet.
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.
OK, you're right - let's announce that properly when the statistics make sense!
5631ab9
to
cac6be5
Compare
On Anyway, bottom line: subjective preference! |
cac6be5
to
0b6e26a
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.
I can approve the changes to coordinates
, but I am not a maintainer of uncertainty
, so I'll avoid merging.
@eteq - do you have time to look at this PR to ensure |
@eteq - ping! This PR touches the |
Since feature-freeze is coming up soon: it would be good if @eteq could review this PR, but failing that I think it should be merged; it does fix some actual bugs. |
There is already approval and no response from @eteq in the past 2 weeks, so I am merging. Thanks! |
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.
invalid_angles = np.any(angles.value < -limit) or np.any(angles.value > limit) | ||
# 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) |
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.
It looks like since this was written there might be a fix in a future numpy for this - might want to add a todo note aobut that
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.
xref numpy/numpy#23247
@@ -311,6 +311,9 @@ def view(self, dtype=None, type=None): | |||
|
|||
# Override __getitem__ so that 'samples' is returned as the sample class. | |||
def __getitem__(self, item): | |||
if isinstance(item, Distribution): |
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 thing it might be better to use duck-typing here - will make a follow-on PR with a proposal
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.
after looking more closely I realize this is a bigger topic that's not worth worrying about right now, because it would be more consistent to do this with all the isinstance
calls in this sub-package, which might have unintended consequences...
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.
Indeed. While usually a big fan of duck-typing, I think in this instance, isinstance
may actually be OK; e.g., I don't think we can count on other implementations to use .distribution
as an attribute... But something to think about nevertheless...
@@ -320,6 +323,30 @@ def __getitem__(self, item): | |||
else: | |||
return result | |||
|
|||
def __setitem__(self, item, value): | |||
if isinstance(item, Distribution): |
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.
(same here as above)
cls.q = cls.a << u.deg | ||
cls.dq = Distribution(cls.q) | ||
|
||
@pytest.mark.parametrize("angle_cls", [Angle, Longitude, Latitude]) |
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.
maybe should add EarthLocation
to this list? Although that might be a whole can of worms.
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.
EarthLocation
will be interesting, since it is already a structured type. That will really need some thought, since presumably each sample then has x,y,z
inside, but we still have to be sure el_dist['x']
works... In other words, I'm pretty sure this will not yet work!
Ooops, my bad on merging too early. I apologize for the extra work. |
No worries, you couldn't know I had just decided to e-mail Erik directly... |
Not yet... 👹 |
This pull request is to address the fact that one cannot initialize a
Longitude
with aDistribution
, which resulted from an error being raised on comparing aDistribution
with a non-void array -- the solution was to override__eq__
and__ne__
(other comparisons are not affected).EDIT: Furthermore, for wrapping,
__getitem__
and__setitem__
needed to be changed to ensure that things likedist[dist < 0] += 360
work. And inLatitude._validate_angles
, one needs to be sure that no assumption of afloat
dtype is made.This part of a hopefully larger attempt to ensure we can use
Distribution
also insideSkyCoord
, etc.Note:
since this is a bug, backporting to 5.2. But perhaps note to 5.0, since really this late it does not seem helpful to fix LTS for this any more.EDIT: while this is a bug, the change tocoordinates
makes me feel that it is better to treat it as an "enhancement" and not back-port.