search

inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

Remove rec_multimap_array_container usage in actx.np.equal #53

Closed alexfikl closed 2 years ago

alexfikl commented 3 years ago

The previous behaviour seems incorrect, since it will recurse and compare even if the inputs don't have @with_container_arithmetic(eq_comparison=True, rel_comparison=True).

inducer commented 3 years ago

I think the existing beahvior is justifiable, particularly if you look at the comment right above there: https://github.com/inducer/arraycontext/blob/ec7d66716276fb64662f576252cdef47adebf6e4/arraycontext/impl/pyopencl/fake_numpy.py#L51-L56

What's the damage if these routines recurse (and are defined to do so)?

alexfikl commented 3 years ago

What's the damage if these routines recurse (and are defined to do so)?

They'll recurse with the proposed change as well if the array container has the proper arithmetic enabled. Of course that doesn't work for object arrays, so this is probably a no-go as is.

I guess this was more of a question than a PR:

inducer commented 3 years ago

Should these comparisons work for array containers with disabled comparisons?

IMO yes. It helps to consider equality comparison and "relative" comparisons separately, IMO. Equality comparison is not well-defined on array context arrays globally: numpy-like ones will do the element-wise thing, while pytato ones will do the "yes/no" structural equality thing. (added a doc todo along those lines to the description) So dispatching to it is asking for trouble. That's why the equality part of this is not popular with me. (Meanwhile, having a reliable way to ask for numpy-style equality comparisons is useful.)

For relative comparisons, both these functions and the operators will do the same thing, so the distinction is less clear. Should they work on types that have rel_comparison disabled? I don't see a reason why not, but I also don't really see a reason why anyone might disable rel_comparison. I'd be OK with checking and yelling if it's disabled for some reason.

Should they broadcast in any way?

Since they represent a way to reliably ask for the numpy-like behavior, IMO yes.