Provide np.broadcast_arrays replacement that allows subclasses#2327
Provide np.broadcast_arrays replacement that allows subclasses#2327mhvk merged 8 commits intoastropy:masterfrom
Conversation
There was a problem hiding this comment.
We may have to include the full Numpy license up here, or in licenses/NUMPY_LICENSE.rst
|
To play the devil's advocate - do we really need this here, or should we just implement a unit-aware broadcast_arrays like the one I originally wrote in eteq#10? I personally think that it would make more sense to just include the simple one I wrote in |
astropy/utils/numpy/__init__.py
Outdated
There was a problem hiding this comment.
Instead of doing this check by version number, we could just check the condition we're trying to fix. For example:
if not isinstance(np.broadcast_arrays([] * u.m), u.Quantity):
There was a problem hiding this comment.
@mdboom - yes, like that. Maybe a bit complicated for MaskedArray, but should be do-able. And then of course we can check for the lower version of numpy that we still support whether a given fix is still required.
|
@astrofrog - I'm thinking slightly ahead, to |
|
@astrofrog - added the numpy license. |
There was a problem hiding this comment.
Should there be an 'else' clause with the numpy imports?
There was a problem hiding this comment.
This function only tests whether the numpy function, which is already imported here, is OK. If it is not, then the subsequent if-statement, which calls this function, will cause broadcast_arrays to be overwritten. So if one imports from this module, net either one gets a correct numpy function, or one patched to be OK.
There was a problem hiding this comment.
Ok, I see now - sorry for being slow!
|
As for the more general procedure: I think we should certainly only have items in Even more generally: is this layout good? My goal would be both to have a fairly obvious procedure of how to go about adding a patched numpy function (use same file & location, must have PR, define function with PR name that tests whether the numpy version is patched, etc.), and also to make sure that it will be relatively straightforward to test whether a patch is still necessary (this is why I export the PR to the top; the idea is that one can check with the lowest supported version of numpy whether a given PR is still I think the above will make it easy to use within astropy; indeed, a follow-up PR would be to use it to remove the fair number of cases where tests for numpy versions are made. A bit less clear is how to use it outside of it. In principle, in |
|
@mhvk - My first reaction is that I think it makes more sense to sequester the actual interface pieces in That attitude assumes all these features will be "compatibility", though, instead of "new feature numpy will never do". In the latter case, I'd advocate it living in an astropy-appropriate place, though: e.g., Regardless, I agree with @astrofrog that waiting to hear from numpy on this first makes sense. |
|
@eteq - Aha, didn't know about that! Yes, I think it makes a lot of sense to move it to One question, perhaps mostly to @taldcroft, there also is a |
|
@eteq, others - would you have a suggestion for where to document the procedure for including numpy compatibility code? I saw that we specifically exclude p.s. travis failure due to |
|
@mhvk - I'm fine with just adding a subsection in the (restarted Travis, hopefully that'll do the trick) |
|
@eteq, others - made a first cut at the documentation; please check whether this makes sense! |
|
Seems fine to me - @mdboom and @astrofrog ? |
There was a problem hiding this comment.
Does PR4622 need to be exposed at the top level here?
There was a problem hiding this comment.
I've been going to and fro about this. Essentially, I would like to make it easy to tell whether a patch is still needed. But probably, it is just as well done by testing whether even for the lowest support numpy version broadcast_arrays is np.broadcast_arrays.
There was a problem hiding this comment.
Yeah -- I think it just clutters the API here. A user that really wants to know can import stride_tricks directly.
There was a problem hiding this comment.
OK, I removed it, and updated the other files + docs to reflect that.
|
Any reason at this point not to move forward on this? |
807685f to
a7f2fa2
Compare
|
@embray - I had not done anything about the Indeed, more generally, would it make sense to import numpy somewhere below astropy, and overlay the changed versions from |
There was a problem hiding this comment.
I don't know if this fix has been included in an actual Numpy release yet, but regardless can we just make this a version check instead?
Considering that we already provide a full replacement here I see no reason to add this additional complication to support the minority of users who would be using a dev build of Numpy.
There was a problem hiding this comment.
As a followup, then the as_strided and broadcast_arrays defined below don't need to be defined under an if statement block. Just define them normally and only import them into the numpy.compat namespace if the version check fails (otherwise just import them straight from numpy)
There was a problem hiding this comment.
I originally had a version check, but @mdboom suggested I replace it... (see his first comment; not sure how to link to it).
One thing I do like about it is that it helps make clear why the feature is there in the first place.
However, on your second point, it would seem to make more sense to import broadcast_arrays and as_strided only when needed, i.e., something like:
if PR4622():
from numpy.lib.stride_tricks import as_strided, broadcast_arrays
else:
def as_strided(...
...
There was a problem hiding this comment.
I sort of disagree there. It's much easier to just have a version check along with a comment explaining why it's there. But it's not too important--I won't insist you change it back now that you've already gone to the trouble :) Though I do think making the import conditional is the way to go.
64cddf9 to
5348946
Compare
|
@embray - it took longer than hoped to get back to this. I ended up leaving what is imported conditional on the function rather than using a version check, mostly because I felt the function should exist anyway, and if it does, one might as well use it... I did rename it to indicate the numpy version where it will be available, though (since the PR# can be found in the doc string anyway). Assuming travis still passes, I think with this it is ready to go in... |
5348946 to
1e5c9d5
Compare
|
@embray - do you think this is ready to go in? |
|
@embray - I'd like to merge this. Would you like to have another look before I do so? |
|
Not really; seems legit. |
Provide np.broadcast_arrays replacement that allows subclasses
In the work on the representation classes for coordinates [1], it was realised that
np.broadcast_arrayscasts all inputs asndarray, leading to again questions whether we should provide "improved" versions of numpy routines (see #1274). In this particular case, there seemed to be no reason that subclasses would be a problem, so I submitted an enhancement PR to numpy [2]. Since even if merged that will not be available in older versions ofnumpy, I also made anastropyversion, as part of a newastropy.utils.numpypackage. I tried to implement a type of organisation that would only override what is not correct, and would also allow us to easily test whether the overrides were still necessary. Probably most needed are comments on this framework![1] eteq#10
[2] numpy/numpy#4622