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
Allow numpy shape functions on ShapedNDArray subclasses #10337
Conversation
Wow, astrobot is good with adding labels! Note that I also moved |
8b1d13c
to
b18e906
Compare
b18e906
to
d0558cc
Compare
d65d0b0
to
c9dff84
Compare
Looking at the PR again, I felt one commit made it harder to review than necessary, so I moved that to its own PR (#10425). For this one, the only real change is in One thing to do in follow-up is to properly implement @eteq, @taldcroft, @astrofrog - review would be most appreciated, it is not as bad as it looks! |
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 gave this a first pass, looks generally good. I don't see any tests of the new methods for Time
, so those should be added unless I'm missing something.
astropy/utils/misc.py
Outdated
from contextlib import contextmanager | ||
from collections import defaultdict, OrderedDict | ||
|
||
from astropy.utils.decorators import deprecated | ||
# For backwards compatibility, import the shape-related stuff. |
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 these symbol definitions for back-compatibility? Are there other uses of ShapedLikeNDArray
outside astropy core?
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 whether it is used outside the core, and even if it was, it would hopefully have been imported from astropy.utils
directly, though it is listed in the API at https://docs.astropy.org/en/latest/utils/index.html#id14. Maybe it makes sense to not copy it over for now, and then possibly re-introduce it if it causes any problems?
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.
But at least I shouldn't leave commented-out code - I've now simply remove it.
c9dff84
to
937c5f7
Compare
OK, comments addressed! I added more tests using |
Note: numpy-dev travis failure is unrelated; see #10432 |
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.
Looks good from my perspective. For the coordinates stuff I have given it a "looks about right" level of review w/r/t tests and basic functionality.
937c5f7
to
bca081e
Compare
@taldcroft - thanks! I realized I had not tested |
bca081e
to
d9ff46f
Compare
Just for functions that only take a single argument and logically deal with shape. Test by converting/reusing earlier tests of np.broadcast_to with self._apply.
Meant also a bit of a rewrite of the test classes.
bb701db
to
c5942c7
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 didn't take an extremely close look at the tests, but did look closely at the code and text, and this LGTM (modulo one very minor docs suggestion)!
docs/coordinates/skycoord.rst
Outdated
`~numpy.ndarray` instances. Similarly, on ``numpy`` version 1.17 or later, | ||
corresponding functions as well as others that affect the shape, such as | ||
`~numpy.at_least1d` and `~numpy.rollaxis`, work as expected. | ||
(The relevant functions have to be explicitly enabled; let us know if a |
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 relevant functions have to be explicitly enabled; let us know if a | |
(The relevant functions have to be explicitly enabled in the ``astropy`` source code; let us know if a |
To clarify that the user doesn't need to enable anything: something has to be changed in the astropy code itself!
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.
Good idea! I'll do it in all the places where I added this... (and fix the docs build at the same time)
c5942c7
to
e3695b6
Compare
Thanks, @taldcroft and @adrn for the reviews. Finally got all the documentation tests to pass (really need an .rst checker for emacs...), and since you both signed off on complementary parts, I'll merge. |
Does the error I see in #10471 look familiar to you? |
Hmm, looks like the tests in |
Partially addresses #8610, though for now only the simplest numpy functions are supported on all
ShapedNDArray
subclasses (representations, coordinates, and times):For instance,
EDIT: while testing, noticed that some functions already worked; e.g.,
np.moveaxis
andnp.rollaxis
(andnp.where
though it returns an objectndarray
).