`cmp`ing fails when attrib is a numpy array

[meereeum]

The generated cmp methods fail if the attrib is a numpy array (of size >1) with

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

this is because:

# equivalent to how __eq__compares fields:
(np.array([47,47],) == (np.array([47,47],) # ValueError
# vs:
(np.array([47,47]) == np.array([47,47])).all() # True

I realize that this can be switched off with cmp=False, but often comparing these attribs is useful!

reproducible example:

@attr.s(auto_attribs=True)
class A:
    arr: np.ndarray

foo = A(np.array([47,47]))
bar = A(np.array([47,47]))

foo == bar # ValueError

the simplest fix might be to trust user annotations about what is/isn't a numpy array and check those separately. or, maybe better, would be awesome to supply a custom cmp function, e.g.

@attr.s
class A:
    arr: np.ndarray = attr.ib(cmpf=lambda f,x,y: f(x,y).all())

where f is the standard cmp function (eq, lt, etc) and x,y are the items being compared.

[hynek]

Since this is the second time numpy equality brought up (ref #409), I think it’s best to keep it open. :)

As a workaround, you can totally write your own __cmp__ by using @attr.s(cmp=False) but then you’ll have to implement all of them.

We’ll have to come up with a nicer way!

[jriddy]

I've found this wildly frustrating myself, but I think this is a problem with numpy breaking normal __eq__ semantics, and attrs actually shouldn't try and solve this. Normally the signature of __eq__ should match object.__eq__'s signature but numpy goes its own way:

# object
def __eq__(self, other: object) -> bool: ...

# numpy
def __eq__(self, other: np.ndarray) -> np.ndarray: ...

This is so you can get the equality product of two arrays easily. And every data scientist I talk to seems to think this is the most natural thing in the world. I deeply disagree with this design decision, but I think the reasoning behind this is clear: in numpy, everything operates on arrays, normal python language semantics be damned!

So in order to solve this issue in attrs, you'd need to have a flexible way of comparing equality on an item-per-item basis...except we already have a good way to do this, and it's the __eq__ methods, which numpy abuses to satisfy its matrix manipulation mania. This allows attrs to leverage tuple.__eq__ to dynamically write obvious code for equality. Making this overridable with some kindof alternative cmp_func attribute or some other solution like that would be overkill and would break the simplicity of the current __eq__ implementation for little gain.

But there are realistically a few ways of handling this:

1. wrap your ndarray, then you can use it normally in other attrs classes:

@attr.s(cmp=False, slots=True)
class Arr:
    arr = attr.ib(type=np.ndarray)
    def __eq__(self, other: object) -> bool:
        if type(self) is not type(other):
            return NotImplemented
        return (self.arr == self.other).all()

@attr.s
class Bitmap:
    raw = attr.ib(type=Arr)
    colorspace = attr.ib(type=str)

Bitmap(Arr(np.array([1])), 'srgb') == Bitmap(Arr(np.array([1])), 'srgb')

2. Use fields + metadata to generate a custom eq method

import operator

import attr
import numpy as np

@attr.s(cmp=False)
class CustomEq:
    label = attr.ib(type=str)
    tags = attr.ib(type=set)
    data = attr.ib(type=np.ndarray, metadata={'eq': np.array_equal})

    def __eq__(self, other):
        if type(self) is not type(other):
            return NotImplemented
        eq_spec = ((f.name, f.metadata.get('eq', operator.eq))
                   for f in attr.fields(type(self)))
        return all(eq(getattr(self, name), getattr(other, name))
                   for name, eq in eq_spec)

Example 2 is more like what I think attrs would have to do (or provide the option of doing) to make these things work in a more general case (with maybe a cmp_hooks=True argument to attr.s), but I think getting that actually right is more tricky than that short example I showed. I don't think I've considered all the angles on that, and it's a lot of work for a specific edge case.

That said, calling numpy an edge case is a bit silly at this point, since the science wing of python is an enormous and important part of the community. But that's why I think option 1 makes the most sense. There have to be meaningful ways to drag their types into the normal python system, and light wrappers make a lot more sense than anything else I've come up with. In fact, you get the added benefit of a place to provide semantic information about things like numpy arrays, which are normally paraded around as naked data structures that reveal nothing of their intent.

I ran into many of these issues at my job and in writing+using zerial (sorry no docs yet), where I'm trying an approach to serialization somewhere between related and cattrs. And I've found that numpy really often doesn't play well with the rest of python. But for an entire class of users, it is the very reason they use python and not something else. So we'll have to figure out a way to do deal with it, and I think wrapping numpy arrays might be the way.

[hynek]

That said, calling numpy an edge case is a bit silly at this point, since the science wing of python is an enormous and important part of the community.

I’m painfully aware of that; I meant it’s an edge case in the sense of not being Pythonic. :|

You’ve kinda nailed how I’d hope people to approach this problem: composition and metadata. The resulting code is far too inefficient for core and metadata is mostly for endusers though, so how should one approach that?

Some thoughts:

1. The information whether an attribute needs special treatment is known beforehand, so it can be taken into consideration while generating code.
2. I guess we'd just add another block of special comparisons before doing the tuple comparison? That way nobody loses anything.
3. So we mostly need to come up with a nice API to define equality-hooks, correct?
4. What about lt/le etc?

[jriddy]

Question: what is so much more efficient about the (self.a, self.b) == (other.a, other.b) approach? I assume you came the conclusion that code generation was necessary because of some benchmark that showed it. Is doing dynamic attribute lookups with getattr what slows things down? I'm curious.

2. I guess we'd just add another block of special comparisons before doing the tuple comparison? That way nobody loses anything.

I could come up with a PR doing that this weekend; it shouldn't be too hard. This use case affects my work a lot, so I could get some real-world feedback with numpy + attrs users pretty quickly.

3. So we mostly need to come up with a nice API to define equality-hooks, correct?
4. What about lt/le etc?

Item 4 complicates 3. If we never had to worry about doing this for ordering, then I would say make eq_fn an argument to attr.ib() and be done with it. Ordering gets more complicated because Python itself makes no assertions about the relationship between the ordering methods:

There are no other implied relationships among the comparison operators, for example, the truth of (x<y or x==y) does not imply x<=y.

I know in the numpy instance, although I want normal equality semantics, I don't really want orderability. The fact that numpy arrays can be stored row- or column-major means that we can't even use index-based priority for comparisons like we can with regular python lists, and even if we could, it doesn't make sense semantically. So in this case, we'd want to opt in to eq/ne but opt out of lt/gt/le/ge. I'll look and get back to you with a PR 😉

[jriddy]

Hmmm, after looking at this, I think the best place to do it would be to overload the type of cmp attribute of Attribute from bool to a Union[bool, CmpSpec], where CmpSpec is a class the describes all this customization. The cmp attribute itself would have a converter to provide some kind of clean API for the end user, so that CmpSpec may not even have to be exposed publicly.

CmpSpec would have to be flexible enough to not only specify alternate comparison functions, but also indicate which comparisons this attribute takes part in, so, say numpy arrays could take part in equality but be neutral for lt/gt, so that they don't affect sort operations. Or float fields could implement a tolerance-based check for equality, but use standard strict lt/gt.

To keep roughly the same efficiency, much of this could be generated in attrs._make._make_cmp except for the most complex cases.

Overloading cmp seems preferable to me to adding another field because 1) cmp is used in very few places in attrs core (only in _make.py it seems), 2) the interactions of cmp and whatever other field would be complex (more complex than cmp and hash, for instance), and 3) it's probably easier to keep a clean, backwards-compatible API this way:

arr_compare = attr.CmpSpec(eq=np.array_equal, ord=False)

@attr.s
class DataStuff:
    name: str = attr.ib()
    arr: np.ndarray = attr.ib(cmp=arr_compare)

I'm working on it.

[hynek]

I think you're on to something with CmpSpec! I had this bug on my mind to switch to enums but thinking about this particular problem, I basically. came to the same conclusion: it would be nice to pass even more info.

This would allow us to finally allow eq-only comparisons which people have been asking for a while.

I wonder how to make this fit into the bigger picture (hence my delay in response, sorry).

We've got ongoing work about customization of Reprs but we're aiming at Union[bool,Callable]. It would be good to take a step back and ask ourselves:

• is *Spec a good name? I literally have no idea.
• Does ReprSpec make any sense?
• Does InitSpec make any sense?
• Does HashSpec make any sense?

That's our current feature switches so we should strive for consistency.

[jriddy]

Yeah, I'm not sure exactly how it would work, but it would make it more . And the CmpSpec class or whatever it's called wouldn't even have to be an attrs-class itself, just duck-type compatible with the interface.

I don't know if Spec is a good name, but at least it's generalizable. The other name I came up with was Comparator 🙁, which I don't particularly like. I think you could use either an add-on word fragment like "Spec" or you could just use normal english agent noun formation, with mixed results:

• Comparer, Comparator
• Representer, Reprator
• Initializer, Initiator
• Hasher, Hashator (ok that one's silly)

On names, I'd talk it over with other people

But I do think it makes sense to use specification objects rather than small unions because it gets harder and harder to manage the values as you have more variants and use them in more places. For example, with the repr, it would make sense to accept a Union[bool, Callable, ReprSpec] and then just convert everything uniformly ReprSpec under the hood:

def default_repr_func(_inst, _attrib, value):
    return repr(value)

@attr.s
class ReprSpec:
    include_this = attr.ib(type=bool)
    repr_func = attr.ib(type=Callable[[object, object], str], default=default_repr_func)

def convert_to_repr_spec(val):
    if isinstance(val, ReprSpec):
        return val
    elif isinstance(val, bool):
        return ReprSpec(include_this=val)
    elif callable(val):
        return ReprSpec(include_this=True, repr_func=val)

But that would only be for making things uniform and allowing for easier future expansion. It almost feels like overkill in the case of reprs. The reason we need this for cmp is because "cmp" combines 2 or 6 concepts, depending on whether we only separate eq/ord or go into the entire rich comparison API.

In any case, much of this wouldn't need to be exposed to the average user, and could be wrapped around for common use cases in some kind of advanced API.

[jriddy]

At any rate, I would like to help out with this comparison implementation, but it may be a week or two before I'm able to follow up on that with an actual PR. Do you guys have a place where you're discussing these other expansions to repr-ability and so forth?

[wsanchez]

@jriddy: #212

[jamescasbon]

I would like a solution to this. I created an evil and naive monkeypatch to try it out:

https://gist.github.com/jamescasbon/b0e1f2113a28e523ff3326d7b93eda19

It does clarify one issue in my mind: array equality is probably going to need policies to be chosen. Sometimes np.array_equal would do but other times we might want all_close with a tolerance.

@jriddy did you get anywhere with a PR?

[jriddy]

No I've been busy. And in most cases this ends up being a less important use case for us. I've been getting by mostly by attribute or class level cmp=False for these things.

The place where it really matters to me is in testing, because we build serialization models around attr.s classes, so I have a whole suite of tests that assert that the serialized and de-serialized object matches the orginal object. This is important to me, but I can build test helpers and fixtures to deal with this use case.

Otherwise...it just hasn't come up the way I thought it would. My research guys...well it never occurs to them that it's even weird that numpy arrays behave this way. Even the ones that are deep into python development (I've got a former matplotlib core dev on my team) think this is totally normal. To me, the type of all the cmp functions is like __eq__(self: object, other: object) -> bool, and that is invariant. But not for them. It almost doesn't make sense to them to define a default aggregate equality predicate because "that's really never what you want to do."

If you can help me clarify the use cases, I think I can work out a solution that could fit into the current implementation. So what are the policies we want for an attribute's cmp property?

[hynek]

As a small update, I think the final idea is brewing in my head, after also flying it by @glyph.

As much as I dislike subclassing, I think it would make sense to refactor the method generation into protocols. Something along the lines of:

class CmpMaker(metaclass=ABCMeta):
    @abstractmethod
    def make_eq(self, cls, attributes):
        pass
    @abstractmethod
    def make_ne(self, cls, attributes):
        pass
    # ...

and each returns a method for cls based on attributes. This would allow you to re-use your implementations, wrap existing ones etc. I suspect/hope that it's a more general solution to the problem. You can also easily disable comparison and just go for eq/ne.

Am I missing something? I'm traveling rn so I haven't gotten around to implement a PoC yet.

[glyph]

@hynek Use Protocol rather than ABCMeta and then you don't need inheritance?

[jriddy]

@glyph It seems more that the pattern of re-use that this would encourage would be inheritance:

class EqOnlyMaker:
    @staticmethod
    def _notimpl(this, other):
        return NotImplemeted
    def make_lt(self, cls, attributes):
        return self._notimpl
    make_le, make_gt, make_ge = make_lt

class MyEqMakerImpl(EqOnlyMaker):
    def make_eq(self, cls, attributes):
        ...

It seems whenever you start introducing method "hooks" like this, inheritance becomes the dominant pattern. How have you guys avoided this with Twisted in recent years? The last I used it, you had to use inheritance everywhere