Codecov Report

Merging #8808 into master will decrease coverage by 0.32%.
The diff coverage is 28.69%.

@@            Coverage Diff             @@
##           master    #8808      +/-   ##
==========================================
- Coverage   86.99%   86.66%   -0.33%     
==========================================
  Files         400      401       +1     
  Lines       59469    59803     +334     
  Branches     1100     1100              
==========================================
+ Hits        51736    51830      +94     
- Misses       7092     7332     +240     
  Partials      641      641

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41930f7...a2ba397. Read the comment docs.

@adrn - thanks for the initial look!

I now also updated the docs, and think this is getting there.

The main issue that I see with this is that it breaks plotting, which means people either have to be very careful with using .value or .to_value(...) everywhere, or use quantity_support(). Unfortunately, it turns out the latter does not work quite correctly with plt.scatter - likely a matplotlib bug.

Note that in numpy 1.17 at least, it is still possible to turn off the whole overrides by setting an environment variable. Possibly, I should document that...

OK, plotting is solved (see #8818), though it is still the case that with this PR, it will become necessary to always use .value or turn on quantity_support().

p.s. One note: I am not yet including functions inside other modules (such as np.linalg) - prefer to leave that for a separate PR.

Very much off topic question, but shall we also drop numpy versions for v4.0? E.g. will it be 1.14+ based on the "last 4 releases" rule?

For a LTS release, it arguably makes sense to insist on a relatively recent numpy version. It might make some sense to exclude 1.14 as well, given that we have work-arounds even for different versions of 1.14; for reference, 1.15 will be a year old by then.

But overall, the work-arounds are all rather small - the only thing that might make a big difference is if we were to require >=1.16, which simplifies erfa and makes __array_function__ possible for all versions.

Anyway, probably best discussed elsewhere!

1.15 will be a year old by then.

1.5 year old. I'll open a new issue, but also put on the tentative agenda of the next dev telecon.

On the larger question of how to review this monster PR: I think we'll almost certainly need to get some experience with this. This is partially why I think it is good to have it in just after a release, so that our affiliated packages can play around with it well ahead of 4.0 (of course, numpy 1.17 still has to be released as well).

@mhvk - there are a few packages that are testing against astropy-dev. Afaik less against numpy-dev, but we also send out an e-mail to astropy-dev that they might want to consider adding that combination to their CI, preferable as a job that also runs from cron.

I suppose this will also close #1273

I suppose this will also close #1273

Yes! 🎆 (and added to top so it auto-closes)

Would this also mean addressing #6897, or just laying ground works for it?

Would this also address #8610?

This will also fix #5842

@mhvk - I don't have the time or courage to give this full review, but I skimmed the crux of the new functionality and it looks great. Just want to congratulate you and numpy for bringing the whole numpy duck array concept to fruition. That's huge! And I do hope that "duck array" sticks, it's much better than any of the alternatives in the NEP.

I agree that this will be great to have! One big-picture question I have - is it conceivable that in future we could migrate to no longer subclassing ndarray as a result of this?

One thing I've noticed so far is that this breaks straight plotting with quantities with different units for x and y (at least plt.scatter) since that internally tries to stick the 2 axes together. We may need to turn on quantity_support by default...

I don't really see any downside to this? For time_support it would be trickier since it has arguments, but I can't think of any reason to not enable quantity_support by default.

Overall this looks great, and I like the way it's implemented. I reviewed the main __array_function__ method and it looks good. I didn't comb through all the tests and specific functions one by one, but I think that if people are generally on board, and this is otherwise ready to go, we should consider merging this soon so that it gets a good amount of testing over the next few months.

@bsipocz - yes, fixes #5842; #8610 is really follow-up. I'll merge it with the other follow-up issue.

@astrofrog, @taldcroft - I agree with the sentiment of merging this soon so we can test it out for a couple of months; the review of __array_function__ itself is the most important; the rest of the code is lots of wrapping boilerplate. I'm covering most with tests, and the only worry there is that I've missed what the units should be for some functions.

@astrofrog - yes, in principle with the combination of __array_ufunc__ and __array_function__, Quantity no longer has to subclass ndarray. But the drawbacks of subclassing have also become less over time - e.g., a lot of method overrides have now disappeared (which we would all have to bring back).

Also, within numpy, the discussion about introducing a new dtype, which could handle units internally, is getting more and more serious. I think that solution makes a lot of sense.

@taldcroft - my personal preference is "array mimic" as it doesn't require knowing the "if it quacks like a duck, ..." etc. But I've gotten little traction. 😄

I'd be happy to see this merged now!

As you wish.

Thank you @mhvk, this is such a great improvement! 🎉 🎆