-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Quantities with structured units #11775
Conversation
👋 Thank you for your draft pull request! Do you know that you can use |
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.
Overall this looks great and extremely useful!
I need to check out this branch and hunt for edge cases because there are a number of complex things happening.
As I'm reading through this now, my two conceptual concerns are: 1) should StructuredUnit inherit from UnitBase and 2) SpecificTypeQuantity, does this work (esp for ones with added equivalencies)?
@@ -3,15 +3,39 @@ | |||
"""Quantity helpers for the ERFA ufuncs.""" |
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.
Are any of these publicly scoped? Nothing has docstrings...
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.
No, they're not. There are some comments about what these functions are supposed to do in quantity_helpers/helpers.py
. I think a good docstring in quantity_helpers/__init__.py
would be an idea... (I've long wondered whether we should use module docstrings for that type of information on implementation; #8930)
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.
Seems reasonable. It's where I write that, when I remember to.
I looked at #8930. I like everything except the custom section title. Numpydoc will complain unless we register in a new section title type. The existing section titles seems sufficient, Notes
or routine listings
can go into details. Or just stuff in the extended summary.
On inheriting from That said, it would be useful to have some type of abstract unit class that defines the interfaces that are actually used (like having a |
@nstarman - structured |
4705573
to
bdf2e4e
Compare
I'm not sure I get the design principle here. The following works >>> u.m.to(u.km)
0.001 But not for a structured unit. >>> u.Unit((u.km, u.km/u.s)).to(u.Unit((u.m, u.m/u.s)))
TypeError: to() missing 1 required positional argument: 'value' I guess I was expecting a structured array of the conversion factors, or maybe a tuple. |
Should astropy/astropy/coordinates/representation.py Lines 481 to 482 in e363e69
have a structured unit? |
astropy/units/structured.py
Outdated
return self._units.dtype.names | ||
|
||
def items(self): | ||
return zip(self._units.dtype.names, self._units.item()) |
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.
Should this use ItemsView
?
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.
And zip
doesn't look good when printed. Maybe consume the iterator into a tuple?
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.
Since both the names and values are tuples and not very long, I agree one might as well make it a tuple. I guess the only downside is that it will slow down some of the code which uses .items()
as an iterator, but the additional 100 ns is probably negligible to everything else...
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 frequently encountered problem Python really need more sophisticated tools for building views. ValuesView, KeysView, and ItemsView should make dict-like views.
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.
Would be nice for repr
, though I am still quite enchanted with the idea that all you need to know about the output of .items()
is that it is an iterable that provides key, value sequences. (If you hadn't noticed already, I love ducktyping!)
operator.methodcaller('_get_physical_type_id'), cls=Structure) | ||
|
||
@property | ||
def physical_type(self): |
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.
Q: should this perhaps be a dictionary the same keys as the unit?
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 is currently a void
that can be indexed with the same keys, but, like for physical_type_id
of the Structure
subclass, which for comparisons ignores the field names. I think that's important. Of course, __repr__
could be overridden, but the the unit input is also as a tuple. Overall, I feel this is most similar to how an individual element of the actual array looks. But I'm not sure...
astropy/units/structured.py
Outdated
return self._units.dtype.names | ||
|
||
def items(self): | ||
return zip(self._units.dtype.names, self._units.item()) |
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.
And zip
doesn't look good when printed. Maybe consume the iterator into a tuple?
Multiplication, division, and exponentiation of structured Quantities is broken >>> pv_values = np.array([([1., 0., 0.], [0., 0.125, 0.]),
... ([0., 1., 0.], [-0.125, 0., 0.])],
... dtype=[('p', '(3,)f8'), ('v', '(3,)f8')])
>>> pv = u.Quantity(pv_values, u.StructuredUnit((u.km, u.km/u.s)))
>>> pv * 2
TypeError: invalid type promotion Also (at least) the addition, multiplication, and division of two structured Quantities is broken >>> pv + pv
UFuncTypeError: ufunc 'add' did not contain a loop... |
Thanks for the further comments! On the ones not in-line:
|
It makes sense that if numpy doesn't allow it, neither should we. But not being able to do |
Essentially, one doesn't 😸 Of course, for things like |
b723597
to
d0e3298
Compare
Let me know when I should give this another look, and perhaps where I should look. The API surface is quite large. |
@nstarman and @adrn - yes, further review would be very good! There are really four things to this rather large PR:
|
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.
@nstarman - yes, further review would be very good! There are really four things to this rather large PR:
- Do the changes to the existing stuff make sense (
core.py
,quantity.py
, parser, etc.)
core.py
andquantity
look good. I'm really not familiar with the parser functions, but what I can parse seems 👍.
Why is Quantity.to_list()
not allowed? If it were, it would also make sense for structured Quantity.
- Is the implementation of
StructuredUnit
OK.
I think I found a bug in view
import astropy.units as u, numpy as np
pv_values = np.array([([1., 0., 0.], [0., 0.125, 0.]),
([0., 1., 0.], [-0.125, 0., 0.])],
dtype=[('p', '(3,)f8'), ('v', '(3,)f8')])
pv = u.Quantity(pv_values, u.StructuredUnit((u.km, u.km/u.s)))
x = pv_values.view(float); print(x[0])
x = pv.view(float) # this shouldn't work
print(x[0]) # this doesn't work but should
namedtuples, should they be treated differently when creating a structured unit? Like a named tuple structured unit has fields names and can only match up with a structured array with the same names? This is almost certainly for a followup.
- Is the use for
erfa
functions OK. This is mostly inerfa.py
, but frankly it makes probably more sense to check that the tests are OK...
I like what I see. Looks useful for the XRepresentation PRs.
I checked out the PR and have been playing around with the examples, trying to get a feel for how it all works. I don't use structured arrays all too often, nor ERFA, so I'm certainly not seeing a lot of the edge cases. From what I can tell, structured Quantities seem quite consistent with numpy's structured arrays, and that's really the point. Hope this helps.
try: | ||
other = Unit(other, parse_strict='silent') | ||
except Exception: | ||
return NotImplemented |
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.
When does this arise? When other is not a unit string?
And can the error be specified, like UnitConversionError or something?
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, when other
is something arbitrary. Here, the goal is simply to check whether the structured unit knows how to deal with other, and I think catching arbitrary exceptions is correct - we should definitely return NotImplemented
on anything that goes wrong, because that means we do not know how to deal with it, and other
should get a chance.
Why is Quantity.to_list() not allowed? If it were, it would also make sense for structured
Quantity.
Obviously, a bit separate from this PR, but what should the output be?
For `ndarray`, it is just a python representation of the array, but how
can we make that for `Quantity`?
I think I found a bug in view
x = pv.view(float); print(x[0]) # this shouldn't work
Yes, agreed that that is strange. In general, for `Quantity`, viewing
with a different `dtype` is very strange, as of course it can be in
numpy (viewing a float as, say, `u4`, generally would not make much
sense). Not completely sure, though, whether it is worth overriding
`.view` for it...
namedtuples, should they be treated differently when creating a structured unit? Like a named
tuple structured unit has fields names and can only match up with a structured array with the
same names? This is almost certainly for a followup.
Indeed. Can address that when needed...
|
Looking at a shaped array, >>> import astropy.units as u, numpy as np
>>> pv_values = np.array([([1., 0., 0.], [0., 0.125, 0.]),
... ([0., 1., 0.], [-0.125, 0., 0.])],
... dtype=[('p', '(3,)f8'), ('v', '(3,)f8')])
>>> pv = u.Quantity(pv_values, u.StructuredUnit((u.km, u.km/u.s))) >>> pv_values.tolist()
[(array([1., 0., 0.]), array([0. , 0.125, 0. ])),
(array([0., 1., 0.]), array([-0.125, 0. , 0. ]))] i.e. >>> (pv_values * u.m).tolist()
[([1., 0., 0.] m, [0. , 0.125, 0. ] m),
([0., 1., 0.] m, [-0.125, 0. , 0. ] m)] and for structured quantities >>> pv.tolist()
[([1., 0., 0.], [0. , 0.125, 0. ]) (km, km / s),
([0., 1., 0.], [-0.125, 0. , 0. ]) (km, km / s)] |
I'm not sure that isn't a bug in
It keeps using tuples for more complicated structures as well. |
Quick note to say sorry for my lack of review -- am away right now, but would very much like to take a close look at this. I think realistically that won't happen until next week, unfortunately -- sorry! |
@adrn - no worries, other than me wanting this off my plate, there is no real hurry! |
Simple to do by storing units in void. Also adjust the erfa quantity wrappers to become independent of field name -- although pyerfa itself does not yet support that.
This since Quantity is no longer imported in structured.py
Plus tests and some cleanup.
Plus a few small changes to ensure those work.
This to avoid possible clash with UnitBase.names.
Also speed up case for dtype. Plus cleanup and more documentation.
Turned out this needed a fix to how physical type IDs were returned, as a plain numpy.void gave a FutureWarning on comparisons, and is sensitive to names, which we do not want to be.
And fix .items() to directly return a tuple, for nicer interactive viewing.
@adrn - thanks for the review and the nice top-level thoughts! I really like the idea of ensuring I'd definitely like to make interactions between structured quantities and representations possible! Perhaps representations could even store their data as such (or at least keep it). I'm also considering using them in the transformations, so that we do not have to do explicit unit conversions any more. But, as you note, for follow up! On the documentation, to me it is not so clear yet what structured quantities will be used for beyond making interaction with pyerfa possible, so my tendency is to keep the documentation as is until we have more to say. Essentially, it will be relatively obvious to people who use structured arrays, and for those who do not know about those, they probably should read the numpy documentation first (not that that is all that great...). |
010725e
to
5055b82
Compare
@adrn - did you have a chance to think about the high-level questions & answers above? My sense is to get this in and leave it all for follow-up... |
@mhvk - sorry for the delay! Some answers below:
Ah, yes, that's a good point...
OK yea, you've convinced me that there are some important / subtle details to think over!
Yes, great!
I think they are more general, but since I have a clearer idea of other use cases, perhaps I should submit a follow-up to make the documentation more general (i.e. it shouldn't hold up this PR!) |
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 get this in so we can start exploring where we might be able to use this internally within Astropy (e.g., in Representation
or Differential
) with enough time to test before v5.0! I'll hold off on merging in case there are any last things you'd like to update, but please ping me again if so for another quick look. If not, feel free to merge @mhvk
Congratz on fixing an issue from 2015! 👏 |
Ever since we made the ERFA ufuncs, I've wanted to be ablve to override all of them, but that was tricky because many require either
Time
input or deal with structured arrays. This PR addresses the latter problem, by implementing aStructuredUnit
, which can be used by quantities with structured dtype. As one can see from the commit history (EDIT: well, not under "commits", but from the list in-line; first commit is from 2018 May 28, almost 3 years ago...), it has taken me quite a bit of time, on and off, but the final implementation is actually reasonably simple - by far the most here is tests, especially of all the ERFA functions that use structured arrays. If this is thought a good idea, I'll complete the ERFA coverage in follow-up in two stages, first for just units, and second also withTime
(latter to be discussed how best to do it).There is also a small documentation page, just to give a sense of how it works.
Opened as draft to get initial feedback. cc @nstarman, who might enjoy a different aspect of astropy.Fixes #3777