-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Better use of __array_finalize__ and __array__ in quantity/longitude/CartesianPoints #1848
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
Conversation
|
My initial changes to the internal use of @mdboom, @astrofrog - we may need to change the names of these methods. It may be worth thinking a bit more generally if it is the right approach. |
|
travis failures are due to timeouts; can ping travis once people have commented. |
|
@mdboom - I realised my chain of PRs was hopeless for making any changes, and so pulled it apart. This one now only addresses better use of |
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 entirely sure about this, but I think this is intended behavior - that is, if you apply a unfunc to a cartesian point, and its unit is not a length, then we don't want it to go through - then it's not a position anymore.
It may be this is explicitly checked for before __array_wrap__, or this was intended to be the check, but either way I think this change isn't necessary/intended?
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 main problem here was calling the initialiser, which does not belong in __array_wrap__ (but rather should be the domain of __quantity_view__ and __quantity_instance__.
But the behaviour also seemed odd: instead of raising an exception, why not fall back to Quantity for calculations that do not give a length (e.g., suppose I want a velocity, why stop me from doing point / (1.*u.yr)?). This is what we do for Angle and its various subclasses as well, and seems least restrictive, so I tried to mimic that behaviour here (with the actual decision on what class to instantiate happening in __array_wrap__, like for Angle, etc.)
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.
Ahh, I understand what you're doing now - thanks for the clarification.
On the final point, I see your point, and I think I'm OK with it.
The one place where it's really important is in initialization, though: we definitely want an exception if, for example, someone does CartesianPoints(xarr*u.Mpc, yarr*u.Mpc,zrr*u.Mpc/u.Gyr), rather than having it silently fall back to a Quantity. But I tested it right now, and that does indeed raise an exception, so I think this is fine.
|
I'm inferring from some of the comments you made above that @mdboom said something about this, but I can't see what... Your changes to |
|
Makes sense to me. |
(viz., __quantity_instance__, __quantity_view__, and __array_wrap__)
|
Just to be sure, since quite a few Quantity PRs were merged, I rebased. Seems all OK. |
Better use of __array_finalize__ and __array__ in quantity/longitude/CartesianPoints
While working on #1839, I realised
Longitude._wrap_anglewas not always updated whenever_unitwas, and I tracked this down to a missing__array_finalize__; with that in place,__getitem__and__getslice__where no longer needed, so it simplified matters. Also changed how numpy scalars are handled: view them as an array using__array__()and then turn the view back into aQuantity.EDIT -- updated title to reflect that I also changed
CartesianPoints(see below)