Split quantity (No __array_function__)

[andrewgsavage]

Just the split into separate sequence and scalar quantities code from #764

[jthielen]

This looks good to me! I would just have two suggested changes:

The first is based on #764 (comment) (item 2) and the one non-__array_function__-related change in andrewgsavage#6:

Would you be able to modify

if not isinstance(other, self.__class__):

in compare to

if not isinstance(other, BaseQuantity):

and add a test for comparison between a QuantityScalar and a QuantitySequence, perhaps similar to the following?

def test_comparisons(self):
    np.testing.assert_equal(self.q > 2 * self.ureg.m, np.array([[False, False], [True, True]]))
    np.testing.assert_equal(self.q < 2 * self.ureg.m, np.array([[True, False], [False, False]]))

Second, would it make sense to get NumPy 1.17 from conda-forge for that build, since it doesn't seem to be available through the default channel?

[andrewgsavage]

Is there anything preventing this being merged?

[hgrecco]

I like this, but this is a big change and I think it would be good to have more eyes before the final merge. @jthielen @crusaderky @shoyer @keewis any other?

[crusaderky]

this method.... does not work. At all. When you try creating an instance of QuantitySequence, you're actually returning an instance of Quantity. Which, in complete frankness, speak a lot of the thoroughness of the unit tests.

Also, as a concept this wreaks havoc in many ways when a-dimensional numpy arrays (ndim=0) start getting involved. Are there thorough unit tests for this use case? When I select a single item out of a QuantitySequence, what do I get?

Are there tests that prove that when a NEP18-compatible wrapper library wraps a QuantityScalar or a QuantitySequence, everything works as expected?

Even if it were a good idea to have two separate classes for when you have a scalar and a vector (and I am definitely not convinced - the people who designed numpy reached the opposite conclusion; did you talk with them?), the whole low level design is problematic - why can't you just have a plain Quantity classs for scalars and a subclass for vectors? Why do you need a mixin that is inherited by a single class? Why do you need to put more and more stuff inside the hack that creates classes on the fly?

Finally, FYI this causes major merge conflicts #880.

Sorry, I'm typically not this harsh, but I in this case I'm strongly against merging without a major redesign and a major review of the unit tests.

[andrewgsavage]

this method.... does not work. At all. When you try creating an instance of QuantitySequence, you're actually returning an instance of Quantity. Which, in complete frankness, speak a lot of the thoroughness of the unit tests.

Quantity will return an instance of QuantitySequence or QuantityScalar depending on the input here. This was done with the intention that the user does not have to change their code to use sequence/scalar. Similarly this prevents having to work out whether the the result of a function (Eg max) is a sequence/scalar when creating a Quantity for the result.

Also, as a concept this wreaks havoc in many ways when a-dimensional numpy arrays (ndim=0) start getting involved. Are there thorough unit tests for this use case? When I select a single item out of a QuantitySequence, what do I get?

Are there tests that prove that when a NEP18-compatible wrapper library wraps a QuantityScalar or a QuantitySequence, everything works as expected?

I expect you'll get the same as what you'd get before this change; Tests are showing no change in behaviour.
You'll get a QuantityScalar. - This could be tested.
@jthielen was using #764 , which is this + array_function

Even if it were a good idea to have two separate classes for when you have a scalar and a vector (and I am definitely not convinced - the people who designed numpy reached the opposite conclusion; did you talk with them?), the whole low level design is problematic - why can't you just have a plain Quantity classs for scalars and a subclass for vectors? Why do you need a mixin that is inherited by a single class? Why do you need to put more and more stuff inside the hack that creates classes on the fly?

Not sure what you mean, a float is not the same class as array[float]. There is some discussion as to why here: #753

A mixin is used to allow other libraries, such as pint-pandas, to inherit.

Finally, FYI this causes major merge conflicts #880.

Sorry, I'm typically not this harsh, but I in this case I'm strongly against merging without a major redesign and a major review of the unit tests.

[shoyer]

With your current implementation, I think you would have equivalent behavior here if you simply removed these __new__ methods on QuantityScalar and QuantitySequence. They are not changing the base-class __new__ method at all.

I agree with @crusaderky that this does have a few questionable design choices:

1. You're generating classes on the fly, which generally is best avoided because it makes things harder to debug. Could you explain why you needed that here?
2. Even if you want the base Quantity class to dynamically create either a QuantityScalar or QuantitySequence, it seems questionable to sometimes return a sequence when a scalar was explicitly constructed (or the other way around). This sort of behavior looks like it could hide bugs.

[shoyer]

Let's discuss the split scalar/sequence issue in #753?

[crusaderky]

Ah, i see. QuantityScalar.__new__ and QuantitySequence.__new__ are never invoked (try putting a breakpoint() in them if you don't believe it). There are two bugs fortuitously cancelling each other.