cc @jthielen and @mrocklin.

I can take a closer look when I get more time, but here's my initial impression:

#6393 was too strict in its dunder operation type check when it comes to scalars since it essentially ignores non-numeric scalars (something that wasn't even on my mind, as all my own use cases are numeric!). This is definitely an issue that needs to be resolved as soon as possible, particularly for the dominant use case of strings (#6611 and #6631). However, the approach of this PR goes too far the other way towards permissiveness, which breaks my primary motivating use case for #6393: Pint compatibility. With #6393, Dask properly defers to Pint Unit and scalar Quantity, however, that is no longer the case for this PR's implementation.

My general take on this is that Dask Arrays need to defer to unknown scalars just like they do to unknown arrays. So, we should probably do a more comprehensive survey of types that Dask should handle out of the box (most definitely strings, but not sure if there are others?), add those to the current check, and then update the documentation to emphasize that other unknown types (both arrays and scalars) need to be registered in order for Dask to operate with them. Perhaps a name other than register_chunk_type should be looked into?

👋 @jthielen thanks for taking a look, I'm obviously open to suggestions. Quick question:

my primary motivating use case for #6393: Pint compatibility. With #6393, Dask properly defers to Pint Unit and scalar Quantity, however, that is no longer the case for this PR's implementation.

Could you please point me to where is this currently tested?

Right now, it isn't being tested in Dask, but in Pint: https://github.com/hgrecco/pint/blob/master/pint/testsuite/test_compat_downcast.py#L95-L111. It's still xfailed, however, due to Pint's slower cadence and its grouping with the similar issue of numpy/numpy#15200 with masked arrays. When writing the comment, though, that was based on just a quick, informal test in the repl.

Perhaps a mock scalar test should also be added to Dask in this regard (don't want it to be Pint-specific, rather than any generic upcast type), or, even better yet, effort be put towards a more robust collection of these array/scalar interoperability tests across the ecosystem?

@jthielen I see, we should definitely add a test that covers that use case.

Now looking at the implementation of Pint's Quantity, I see why this PR would no longer delegate to Quantity. It's a bigger question of whether Dask should or should not delegate to "unknown scalars", and more importantly how should that change be introduced in Dask (because current implementation (to delegate) is a breaking change (that was already released)). Is there an agreement on this @TomAugspurger @mrocklin @jthielen ?

@jthielen added a simple test to validate/cover your original issue (we can obviously make this look better, I just wanted to have this covered in this PR)

class TestScalar:
    def __radd__(self, other):
        return TestScalar()


def test_unknown_scalar_delegation():
    # This PR does not delegate to unknown scalars:
    assert type(da.from_array([1, 2]) + TestScalar()) == da.Array
    # Previous implementation would be:
    # assert type(da.from_array([1, 2]) + TestScalar()) == TestScalar

@TomAugspurger

Some context:

• prior to Add simple chunk type registry and defer as appropriate to upcast types #6393 afaiu all scalars (non array-like) were handled by Dask
• after Add simple chunk type registry and defer as appropriate to upcast types #6393 "scalars" that are [Number, list, tuple, np.generic, *dask.dataframe.core.Scalar*] are handled by Dask, the rest gets delegated (this includes str, datetime, custom user types etc)

There are at least 2 solutions:

• treat unknown scalars the same way we currently (post Add simple chunk type registry and defer as appropriate to upcast types #6393) treat unknown arrays (-> delegate to the unknown scalar), which would involve defining a registry of known scalars that Dask would handle (potentially pandas' isscalar) (essentially Fix scalar dispatching, fixes #6631 and #6611 #6632 (comment))
• go with this PR's implementation, and handle arrays according to the defined hierarchy, but scalars/non array-like by default are handled by Dask

There are obviously pluses/minuses to both. Looking at this from the user perspective, in my opinion, scalar handling before #6393, made more sense (which this PR "reverts" to). That said we should probably still allow for a custom scalar (like Pint's Quantity) to notify Dask that it can/wants to handle an operation (via for example of some kind of evidence/protocol inside of the custom scalar, thus delegating the responsibility to the custom scalar's class/definition). I might be wrong, but in my mind these kind of scalars would be special cases, and therefor should be treated that way. What do you think?

Do we have one other option? We could try manually deferring to other by calling its dunder method. We'd take over only if it's not implemented. Essentially re-implementing Python's logic manually. There are probably edge cases I'm not considering.

I'm somewhat pessimistic about a system that relies on third-party libraries registering things, at least specifically with Dask.

Looking at this from the user perspective, in my opinion, scalar handling before #6393, made more sense

My hope is that this doesn't affect most users at all :) If we ironed out all the issues with the current implementation by having Dask take over for more types, would the two implementations be equivalent from your point of view?

👋 @TomAugspurger

I'm somewhat pessimistic about a system that relies on third-party libraries registering things, at least specifically with Dask.

Just to be clear, in my mind that would not be a "manual" registration (akin to the current design of register_chunk_type), but rather an evidence/protocol that the custom class would have to implement. I actually see you suggestion as an implementation of that (where the custom class has to implement dunder method for (Dask) array), I was actually thinking about something more explicit, but I like your idea as well! There are some things I am worried about tho:

• Dask would essentially reverse the python order of resolution, for __op__, it would first try to delegate (I assume to __rop__ of the custom scalar), and only if it gets NotImplemented it handles the __op__
• do we need some kind of special consideration for in-place ops?

Quoting Guido van Rossum from this issue:

Originally my design was just

1. try left arg's __op__
2. if that returns NotImplemented, try right arg's __rop__

That's simple to explain and usually works. But when thinking through edge cases, we found that if the right class is a subclass of the left class, this gives unexpected results, so we added the exception. if the right class subclasses the left class and it overrides __rop__, try that first

I honestly believe that I didn't realize that it could ever matter if the right class subclasses the left class without overriding __rop__. And nobody else realized it either.

I think we decided on the "right class must override __rop__" condition in part to preserve the original, simpler rules for cases where we thought they would suffice, in part because for most users trying __rop__ before __op__ is unexpected, and in part because we didn't realize that it mattered.

And it only matters if the base class uses something like type(self) or self.__class__, which as a design pattern is sometimes dubious and other times useful.

Also note that the language Stephan quoted is somewhat different: it uses "provides" (__rop__) instead of "overrides". I could imagine that this, again, was a difference too subtle for most folks to notice.

If I had to do it over, I agree that if the right class subclasses the left class, we should always try __rop__ first. But now that this has been an ingrained behavior for so long I worry that it will break existing code, esp. in cases where there is an asymmetry between __op__ and __rop__ and the class author did not expect __rop__ ever to be called first.

afaiu #6393 follows that design (+ NEP-13) for array-like (with explicit downcast hierarchy, which is great and makes sense). For scalars, there is no explicit hierarchy, so it seems like we should follow the pythonic way (subclasses before superclass, left to right)? We could diverge, I'm just no sure what kind of dragons to expect once we cross that threshold?

Another alternative could be for scalars to "force" their own op on (Dask) array by setting __array_ufunc__ to None, according to NEP-13:

class TestScalar:
    __array_ufunc__= None

    def __add__(self, other):
        # other could be a Dask array, that this implementation knows how to handle
        return TestScalar()

    __radd__ = __add__

@pytest.mark.parametrize("arr", [da.from_array([1, 2]), np.asarray([1, 2])])
def test_unknown_scalar_delegation(arr):
    assert type(arr + TestScalar()) == TestScalar
    assert type(TestScalar() + arr) == TestScalar

The test above works, but actually NOT for the reasons that it should (if you step the debugger you will understand why, more on that below). No change in Dask is needed (apart from this PR, which reverts scalar handling to priori to #6393), and we delegate the responsibility to the scalar classes. BUT it seems Dask Arrays do not follow NEP-13 when to comes to handling classes with disabled __array_ufunc__ (by setting it to None), the check_if_handled_given_other's wrapper should probably have:

        try:
            if other.__array_ufunc__ is None:
                return NotImplemented
        except AttributeError:
            # there is no info about __array_ufunc__, so we handle it the python way (l -> r)
            pass

Which is slightly modified recommended solution from NEP-13.

Also I'm wondering, how many custom scalars apart from Pint's Quantity etc, would actually need this support? I'm essentially wondering how important is this feature to solve right now? For example, we could revert the scalar (not the array-like) handling from #6393, and open a completely new issue on Dask to discuss the design for custom scalar resolution in depth, essentially solve the pressing issue now, and gradually improve without affecting users. The only "real" use case I'm aware of is the Pint's dask_arr * quantity (this doesn't work for numpy either btw), which can be worked-around by depending on python operand order quantity * dask_arr. I'm not saying we should not fix it, but I'm saying maybe it's not as time sensitive as the issues that #6393 has already introduced.

Btw on Pint's Quantity, it turns out that Quantity already has __array_ufunc__, to my understanding (and I barely looked at the codebase), Quantity sometimes acts like a scalar and sometimes like an array-like, this introduces complexity, but afaiu if Dask was to follow NEP-13 (__array_ufunc__ check on scalars), that complexity is delegated to Pint (where it belongs), not Dask.

Thanks for that analysis @ravwojdyla. It'll take me some time to digest it :)

Input from others (possibly @shoyer if you have time, since you've thought about this a lot) would be welcome.

I agree, thank you for the thoughtful analysis @ravwojdyla! I must admit that my resistance to the change of this PR is that one of the main reasons I put in the effort to make #6393 happen is so that dask.array.Array * pint.Quantity and dask.array.Array * pint.Unit are always pint.Quantity, since that is the idiomatic way to construct a Quantity, so to see that go away so quickly after waiting a long time for #6393 is disappointing. I would totally understand though if the consensus is to revert that behavior in the short-term so as to satisfy the needs of the broader user base, although I would hope that another solution could be found (such as just adding the common scalar types to the handled type check of #6393, or adding in the reversed-resolution dunder op check)

That being said, my impression of this now is that the main point of contention/disagreement is that I haven't considered scalars to be any different than arrays when it comes to the type casting hierarchy dispatching of NEP-13/18, whereas you are considering them to be fully distinct.

The main reasons I think that scalars should be treated under NEP-13 style dispatching just like arrays (particularly when one type is an array and another is a scalar) are:

1. After much discussion over in Pint and input from people like @shoyer and @crusaderky, the consensus is that Pint's single class for both scalars and arrays is the preferred design (Split in two Quantity Classes for Scalar and Array objects hgrecco/pint#753 (comment) particularly, but both Split in two Quantity Classes for Scalar and Array objects hgrecco/pint#753 and Shall we create a QuantityArray class? hgrecco/pint#1128 allude to additional discussion) and separate classes are a bad idea. This only really makes sense in the ecosystem at large if scalars and arrays are treated the same when interoperating with Pint.

2. Commutativity of operations like addition and multiplication is paramount. NEP-13 makes this "just work": right now (i.e., with Add simple chunk type registry and defer as appropriate to upcast types #6393), these operations with xarray DataArrays, Pint Quantities (both arrays and scalars), Dask Arrays, Sparse COO arrays, and NumPy ndarrays are indeed commutative and result in the proper type higher on the heirarchy. For the case of Pint Unit (which is always a scalar), we have all the following (as is expected/proper behavior):

xarray.DataArray * pint.Unit == pint.Unit * xarray.DataArray == xarray.DataArray(pint.Quantity)
dask.array.Array * pint.Unit == pint.Unit * dask.array.Array == pint.Quantity(dask.array.Array)
sparse.COO * pint.Unit == pint.Unit * sparse.COO == pint.Quantity(sparse.COO)
numpy.ndarray * pint.Unit == pint.Unit * numpy.ndarray == pint.Quantity

This is only really broken when downcast types don't respect the type casting heirarchy (Dask pre-#6393, MaskedArrays)

Unless I'm missing something, since composition is preferred over inheritance, these are all essentially wrapper classes rather than subclasses, which makes it difficult to ensure commutativity with the standard Python behavior you mentioned (subclasses before superclass, left to right).

3. For custom type implementers/users, it makes sense to just have one way to make it known to Dask as a type Dask should handle in its interoperations, rather than one way for arrays (register_chunk_type, an opt-in registry as settled on in Add simple chunk type registry and defer as appropriate to upcast types #6393 after much discussion) and another for scalars (an opt-out registry or custom protocol as discussed here)

Hopefully this helps move the discussion along, and hopefully we can find a resolution that works for everyone's needs!

@jthielen thanks for the feedback. To reiterate, I'm not against Pint's use of * to construct a Quantity (since I have never used Pint, I don't have an opinion about that construct). But I do worry about effectively "broken" Dask master, and fixing it should be the priority. That could mean creating a separate issue to discuss scalar handling further if necessary.

@jthielen @TomAugspurger on the 2nd glance at Pint's Quantity, and remembering something from NEP-13, maybe there is a middle ground, if:

def check_if_handled_given_other(f):
    @wraps(f)
    def wrapper(self, other):
        if hasattr(other, "__array_ufunc__") and other.__array_ufunc__ is None:
            return NotImplemented
        elif (
            hasattr(other, "__array_ufunc__")
            and not is_valid_array_chunk(other)
            and not is_dask_collection(other)
        ):
            return NotImplemented
        else:
            return f(self, other)

    return wrapper

This implementation of the check_if_handled_given_other makes Dask Array more NEP-13 compliant given:

1. If other.array_ufunc is None, ndarray returns NotImplemented. Control reverts to Python, which in turn will try calling a corresponding reflexive method on other (e.g., other.rmul), if present.

2. If the array_ufunc attribute is absent on other and other.array_priority > self.array_priority, ndarray also returns NotImplemented (and the logic proceeds as in the previous case). This ensures backwards compatibility with old versions of NumPy.

3. Otherwise, ndarray unilaterally calls the corresponding Ufunc. Ufuncs never return NotImplemented, so reflexive methods such as other.rmul cannot be used to override arithmetic with NumPy arrays if array_ufunc is set to any value other than None. Instead, their behavior needs to be changed by implementing array_ufunc in a fashion consistent with the corresponding Ufunc, e.g., np.multiply. See List of operators and NumPy Ufuncs for a list of affected operators and their corresponding ufuncs.

A class wishing to modify the interaction with ndarray in binary operations therefore has two options:

Implement array_ufunc and follow Numpy semantics for Python binary operations (see below).

Set array_ufunc = None, and implement Python binary operations freely. In this case, ufuncs called on this argument will raise TypeError (see Turning Ufuncs off).

First point is fulfilled by the if branch. Second point, __array_priority__ is replaced by the explicit downcast registry check in the elif branch. Third point by the else branch.

Above implementation passes test_dispatch +:

@pytest.mark.parametrize("arr", [da.from_array([1, 2]), np.asarray([1, 2])])
def test_unknown_scalar_delegation(arr):
    ureg = pint.UnitRegistry()
    assert type(arr * ureg.meter) == ureg.Quantity
    assert type(ureg.meter * arr) == ureg.Quantity

def test_operator_on_scalar():
    a = da.from_array(["a", "b", ".", "d"])
    # Validate/fix 6631
    assert_eq(a == ".", [False, False, True, False])
    # Validate/fix 6611
    assert "b" in a

@ravwojdyla That fix looks almost perfect to me! It aims to preserve the hierarchical dispatching for any array/scalar implementer of __array_ufunc__, which seems appropriate, while also switching over the default handling for non-__array_ufunc__ implementers to being on Dask's end, as needed for #6631 / #6611. The only problem I can see with it as written is duck Dask Arrays: types that implement both __array_ufunc__ and the Dask Collection interface, but are not Dask Arrays themselves (e.g., Pint Quantities and xarray DataArrays that wrap Dask Arrays). However, I think that should be easy to resolve by replacing the is_dask_collection check with a more explicit check for Dask's internal types?

To me, more detailed discussion about scalar handling is only needed if a use case for a non-__array_ufunc__ implementer needs to take priority over dask.array.Array in Python binary operations arises...and I can't really see that ever being the case?

PTAL @TomAugspurger @shoyer @jthielen

I'm happy to defer to others here.

I've taken a brief look through this and I'm also going to pass I think. If no one shows up to review I'm ok to trust @ravwojdyla and @jthielen and merge . However, I think that it would be good to get more perspectives in here if possible. The code changes here are not large.

In addition to @shoyer I'm also going to ping @pentschev , who I think has a good understanding of cross-array interactions.

The most recent updates generally look good to me! Just one suggestion for the modification to an existing test (see inline comment), and one question: given the refactor of binops and __array_ufunc__ checks to use _should_delgate (which was a great idea), should __array_function__ do likewise, or leave it as-is? If it is refactored, we could get rid of is_valid_chunk_type and just use is_valid_array_chunk by way of _should_delgate.

@jthielen I suggest we keep NEP-18 separate for now, and create a couple of issues:

• consolidate NEP-13 and NEP-18
• binops should delegate via __array_ufunc__ (which is the NEP-13 recommendation, but not part of this PR)

Thanks for looking Peter.

I'll plan to merge this later today (@jcrist was maybe going to look if he had time). We'll hopefully have a release tomorrow that should include these changes.

The implementation looks good to me, but I confess I don't have a strong understanding of __array_ufunc__ semantics to know if we're doing the right thing. I trust the people that did this work though, and the tests look good.