Quantity __array_function__ support for histograms, arraysetops functions and apply_{along, over}_axis #8900
Conversation
mhvk
added
units
Affects-dev
This comment has been minimized.
This comment has been minimized.
mhvk
force-pushed
the
quantity-array-function-setops
branch
from
fe7c5ae
to
14aad50
Compare
|
Oops.. fixed. |
mhvk
force-pushed
the
quantity-array-function-setops
branch
from
14aad50
to
7e524b4
Compare
mhvk
changed the title
|
No idea what's up with the coverage - I checked locally and it looks OK. |
|
@mhvk , did you rebase on the latest |
|
No, I did not since this is a sequence of PRs; if I rebase one, I'll probably have to rebase all of them, even though they do not have merge conflicts. |
mhvk
force-pushed
the
quantity-array-function-setops
branch
from
2eb5412
to
6432a5b
Compare
mhvk
changed the title
|
Now included all further updates to |
There was a problem hiding this comment.
Looks good to me!
I have a slight concern that given the amount of code we are copying from Numpy, it's not clear how we'll be able to easily keep track of fixes to bugs in the copied Numpy code, but I'm assuming that we can just cross that bridge if our users run into those bugs. Anyway, nothing to act on now, but just want to raise the point that we'll have to make sure this is reasonably easily maintainable in future.
| assert_array_equal(outdd_h, expecteddd_h) | ||
| for o, e in zip(outdd_b, expecteddd_b): | ||
| assert_array_equal(o, e) | ||
| # Bit tired, so just copying the tests from histogram2d above. |
There was a problem hiding this comment.
Is this something you want to address before merging?
|
@astrofrog - thanks for the review. I agree about the implementation, and sort-of tried to have more in-depth testing for functions where we, effectively, forked. I don't really know what to do beyond that - in particular, added functionality over at numpy (new keyword arguments, new meanings) will need to be propagated. One possibility at least for new arguments could be to ensure not to use I'll look at factoring out the histogram tests. In the meantime, any idea about the circle-ci failures? |
with a quick glance they look to be the same as in master, and I share @pllim's suspicion in #8934 that they are likely pytest5 related failure. I'll try to find time to look into that during the weekend, unless someone else will be quicker :) |
|
OK, if master has the same errors, I think it is safe to merge this one, so I'll go ahead and do that. Thanks, all. |
fixes the regression noted by @StanczakDominik in #8825 (comment)
It is not completely clear how useful set operators are for multi-argument functions where units are converted, but I decided to include them anyway. User beware and all that.
Note that this includes #8884 and #8894 since otherwise I get failures. Just look at thelastthird-but-last commit only.EDIT: I now also included coverage for the unrelated
np.apply_along_axis(which worked anyway) andnp.apply_over_axes- I did not want to make yet another PR on top of this stack of three...EDIT: now includes updates also for histograms, so all the additions are together.