Distinct instances of the same node do not compare equal

[borellim]

Example input:

n1 = load_node(42)
n2 = load_node(42)
print n1, type(n1)
print n2, type(n2)
print n1 == n2

Output:

uuid: ddf9a6de-56bf-4783-96d2-33400df20f43 (pk: 42) <class 'aiida.orm.data.upf.UpfData'>
uuid: ddf9a6de-56bf-4783-96d2-33400df20f43 (pk: 42) <class 'aiida.orm.data.upf.UpfData'>
False

Tested with types UpfData, WorkCalculation, PwCalculation.

EDIT: also tested with more "basic" types aiida.orm.data.str.Str and aiida.orm.data.int.Int. For these, some more discussion is possibly required: should the PK/UUID be compared, or the actual data themselves?

[ltalirz]

@sphuber You mention here that there may be reasons for not implementing the behaviour asked for in this issue.
Would be curious to know what these reasons were ... perhaps @giovannipizzi can also comment

[giovannipizzi]

Mmm not sure.
I feel that we should not compare the content (otherwise the behaviour depends on the data type).

However, I cannot think now to a real problem why implementing comparison by PK (or even better by UUID - in case in the future we support loading at the same time multiple profiles, and different nodes might have the same PK).

I suggest to implement it unless we realise there's a problem.

[sphuber]

Mmm not sure.
I feel that we should not compare the content (otherwise the behaviour depends on the data type).

I think this would be mostly very difficult to implement correctly and be sure it always behaves correctly.

However, I cannot think now to a real problem why implementing comparison by PK (or even better by UUID.

Since the UUID is what we consider to uniquely identify a node in our data model, I think it is OK to base the equality operation on this, so I am also in favor implementing this.

I suggest to implement it unless we realise there's a problem.

The only thing I can think of is where someone in code may change the UUID of an unstored node to an existing one manually and then do a comparison, but I don't think this should pose a problem. This example is outlandish and storing the node would fail now that there is a uniqueness constraint on the UUID column.

[ramirezfranciscof]

@ltalirz @giovannipizzi @sphuber One thing I just discovered with this, is that currently there are some data nodes that use a different criteria to establish the concept of equality. I don't know the full extent of this, but I think this originates from the BaseType inside of orm/nodes/data/base.py, so it should apply to all nodes that mock python data types. For example the Int type will check if the value that is stored is equal.

This now poses a series of problems:

1. Changing the existing behaviour would break backwards compatibility.
2. Having both behaviors depending on the type of node would lack consistency.
3. Using value as criteria in python-like nodes allows for an intuitive/natural handling of those nodes but can't be extended to other node types.
4. Using uuid as criteria is extensible and consistent, but we would loose the "natural" handling of these basic node types.

I'm unsure on how to proceed. I am inclined towards going for uuid in all cases and just biting the bullet on 1 and 4, but I wanted to hear what you guys think. Maybe we should discuss this in the next aiida meeting...

[sphuber]

Very good catch. This does indeed complicate things. I definitely agree that we should have one or the other and not a mix of behavior that depends on the type. However, this would lead us naturally to only ever allowing to do comparison based on UUIDs. This would mean we just need to deprecate the current implementations of __eq__ and say that as of v2.0.0 this will work differently and only compare UUIDs. But I agree that this would be good to discuss in a meeting.

[greschd]

Yeah, I have to say I'm quite opposed to changing the equality behavior of the numeric types. That can break existing code in quite complicated and hard-to-find cases.

What's the harm in comparing by UUID just as a fallback, for the types that don't have a custom comparison function? After all, that is e.g. how the built-in Python types do it (compare by value, and otherwise by id()).

[sphuber]

What's the harm in comparing by UUID just as a fallback, for the types that don't have a custom comparison function? After all, that is e.g. how the built-in Python types do it (compare by value, and otherwise by id()).

Well, the risk is that if it is not consistently implemented by (all) data plugin types, it may give unintuitive behavior. For example, the Dict node doesn't implement the __eq__ method like the base types and so Dict(dict={'a': 1}) == Dict(dict={'a': 1}) will return False even though that is not what one would expect based on how the base types behave. Especially if you draw the parallel with behavior of Python built ins where dictionaries do respect that equality.

Of course we could then implement this for Dict, but then the question becomes, for which node types should we implement and for which shouldn't we. This line may be difficult if not impossible to be drawn.

Anyway, not really sure which would be more frustrating: not having all data plugins behave the same, or not being able to compare base data types based on their value.

[greschd]

Anyway, not really sure which would be more frustrating: not having all data plugins behave the same, or not being able to compare base data types based on their value.

I think most frustrating would be the behavior for the NumericType suddenly changing..

Yeah I agree inconsistency in which Data nodes implement "deep" comparison isn't ideal - but again, that's essentially the same behavior Python itself has. It's definitely not always ideal, but at least it's something you need to be aware of anyway when programming in Python.

[sphuber]

but again, that's essentially the same behavior Python itself has. It's definitely not always ideal, but at least it's something you need to be aware of anyway when programming in Python.

Hmm, fair enough. Maybe the fallback on UUID is fine then. Let's see what others think

[ltalirz]

Fallback to UUID when specific equality method is missing sounds good to me.

[ramirezfranciscof]

Yeah I agree inconsistency in which Data nodes implement "deep" comparison isn't ideal - but again, that's essentially the same behavior Python itself has. It's definitely not always ideal, but at least it's something you need to be aware of anyway when programming in Python.

So it might be true that Python itself does this, but I'm not sure I would think of these kinds of things as python "fortes". I mean, I understand the complexities behind the behavior and perhaps this was the right choice for them, but I'm not super convinced this means it is the best for us. Like, I already thought that the == vs is difference was confusing enough, now add the fact that for some objects they might actually be using one for the other and it triggers my "nobody can be trusted" response =P.

For example, one thing to consider is that if we set this fallback behavior to uuid equality, if we then decide that another node type would benefit from having a way to easily evaluate content equality (checking if you already have a pseudo for example), the change is no longer a simple feature addition but a backwards compatibility breaking change. And I feel that this kind of breaking is specially annoying to the user when there is not a strong reason for the original criteria nor for the change (particularly since there is a high probability they will be "clashing" with these things, i.e. realizing these differences mostly through errors when trying to use them).

I think that if we are going to decide there is more worth in the concept of "equality of content", we need to be consistent and have this be the expected behavior for all node types. Actually, now that I think of it, this might even make more sense, since "equality of uuid" is just evaluating node1.uuid == node2.uuid so it is really not a super useful feature, but offering a pre-implemented "equality of content" might actually save the user the need to code complex comparison mechanisms themselves. We can still have a fallback mechanism (both for cases where it might not make sense/be possible to compare content and cases were we just haven't figured out yet how to best do it), but I would much rather that be a raise with the proper error message than an unexpected behavior that has a considerable chance of going undetected.

[ltalirz]

I think you make a valid point. The UUID fallback would mean that it becomes difficult to interpret the meaning of node1 == node2 in the code, since it can mean qualitatively different things depending on the node class (equality of content vs equality of uuid).

Since raising an exception would also be a breaking change, I'm just wondering: don't we already have a better fallback in the hashing mechanism? Could a comparison of the node hash be used as a fallback instead, if a specific method is not implemented?

[greschd]

is just evaluating node1.uuid == node2.uuid so it is really not a super useful feature

I agree it's not a critical feature, but one nice thing this enables is using nodes in built-in data structures, e.g. set.

don't we already have a better fallback in the hashing mechanism? Could a comparison of the node hash be used as a fallback instead, if a specific method is not implemented?

I have a feeling this could have some even more puzzling corner cases 😅 Off the top of my head:

• hashes only take into account the immutable attributes of a node
• the whole rehashing story doesn't mesh well with this (would need to rehash everything when updating code that touches hashing)
• for float types, the hash is more akin to an "almost equal", due to wanting to avoid unstable hashes when going from in-memory to DB

So.. it's not quite as easy as it seems, and the design considerations going into the hashing don't translate well to this problem I think.

Since raising an exception would also be a breaking change

It definitely shouldn't raise an exception - the Thing To Do™ in such a situation would be return NotImplemented, so that Python can go its normal fallback route. That is, it would return id(self) == id(other).

[greschd]

Like, I already thought that the == vs is difference was confusing enough, now add the fact that for some objects they might actually be using one for the other and it triggers my "nobody can be trusted" response =P.

I think going into the reasons for this might be useful:

• the __eq__ is syntactically any equivalence relation
• the is comparison is "is exactly the same Python object"

Both operations are "default to False": for example a == b will be False if neither a nor b know how to compare to each other.

In that light is is a stronger requirement that __eq__: If a and b reference exactly the same Python object, they are also equivalent. Otherwise __eq__ wouldn't be an equivalence relation (that's the reflexive property).

So you can thing of __eq__ as "strict equivalence, but without guarantee there won't be false negatives".

---

Here, we have three different equivalence relations (in order of strongest to weakest):

• (a) two nodes are the exact same Python object (current fallback)
• (b) two nodes have the same uuid
• (c) two nodes have the same content

If two nodes fulfill (a), they will fulfill (b) and (c). If they fulfill (b) they will also fulfill (c), but not necessarily (a).

Currently, we implement (c) and fall back to (a) where that's not available. The question here is if we want to fall back to (b) instead.

[ramirezfranciscof]

Ok, yes, that last comment gave some things to think about. I'm still wary about the unreliability of where it will do (c) or (a/b), but I guess that if you present it like that ("use this when you want to discern repeated content, with the caveat that you might get repetitions when we don't know how to check that content") it could be ok. I still need to process it a bit.

[giovannipizzi]

Very interesting thread, and thanks for all useful discussions.
I have to admit that while reading it, at every comment I thought "ok we should do this" and at the following "ah right, wait!"...

BTW, just to stay on the topic of what python does, just for fun:

# a bit weird to me, but ok...
3 == 3.00 # True

# Very good
3 == 2.999 # False

# For floating-point lovers ;-)
3 == 2.9999999999999999999999999999999 # True

# Luckily this is not javascript!
3 == "3" # False

# numpy makes things very strange (but turns out to be very useful in practice)
numpy.array([1]) == [1] # array([ True])

So the list of things that can happen is quite varied.

Getting back to serious stuff.
Now, on the raising-an-exception topic: while in general I agree that raising an exception when this is never done is not a good idea, I just point out an example where this is done by numpy (ok, not by python), for good reasons:

In [17]: bool(numpy.array([1, 2]))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-17-c43772ca109d> in <module>
----> 1 bool(numpy.array([1, 2]))

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

Now, I see reasons for the various suggestions:

• I think most people expect node1 == node2 returns true if they are the same node (we even have duplicate issues about this :-) ). Of course, if we don't want to have this behaviour (now), we can tell people 'use node1.uuid == node2.uuid', like numpy says for bool.
• While I follow the reasoning by @greschd in this comment, I am still not 100% convinced of how many people would find it intuitive, and not getting bitten by the behaviour (in the end, I don't think we can expect people to search in the docs how == works in AiiDA before using it, even if we were to document it very clearly...).
• I think the main reason of the issue is that the Int, Float, ... nodes try to mimic the corresponding int, float python types. I am still a proponent of dropping this feature, because it creates so many issues (this one, the risk of passing a node instead of a number and making calculations 6 orders of magnitude slower - there is an issue for that), etc., and anyway we are not covering all possible operators for all "base" types. Just one simple example:

  In [22]: l = List(list=[1])
  
  In [23]: l2 = List(list=[2])
  
  In [24]: l + l2
  ---------------------------------------------------------------------------
  TypeError                                 Traceback (most recent call last)
  <ipython-input-24-e5e78452f0eb> in <module>
  ----> 1 l + l2
  
  TypeError: unsupported operand type(s) for +: 'List' and 'List'

  Or, even wanting to stay really just with Bool, Int, Float, ..., neither we map all base type methods, e.g. is_integer() for floats does not exist for Float, so anyway one cannot always just reuse Float instead of float in some cases (also for performance as mentioned earlier).
  So, since we are now starting to discuss a 2.0, this would be a good moment to decide if we want to keep this feature or not. @greschd seems to prefer to keep it, but if we drop it completely (and we start deprecating its use already now) I think this will give time to developers to adapt their plugins, and the fix is easy (use node.value instead of just node (like now doing List(list=[1]).get_list()), or get_dict() for dicts).
  This will make the user/developer more conscious of what they are doing. And save us from complex discussions like this in the future.

With this suggestion, I would still wait 2.0 (or even 2.1?) to implement then the equality of nodes based on UUID only (what I think is more intuitive).

Comments welcome, also from workflow writers who might be using the overloaded classes a lot maybe? (But again I think there it's in the end still better/easier to know the type and use .value() explicitly?)
Examples where the .value syntax would be much more cumbersome in currently implemented code is welcome!

[greschd]

Yeah, I think I can get on board with dropping the numeric behavior of Int, Float, etc., with a sufficient deprecation period. I know I tend to use it when writing one-off "test" calcfunctions for example, but maybe less so in actual production code (I'd have to check).

Since there's no "strong" reason to drop it, maybe it would also be good to get plugin developers' feedback.

Some points I'd like to make:

• I think it would be beneficial to keep the to_aiida_type helper around, since users will otherwise have to implement this themselves - which is error-prone. See NumericType behaves unexpectedly in operations with different types #596 and Fix numeric type arithmetics #597 (wow, three-digit issue numbers 😄)
• The Bool -> bool conversion (added in Fix Bool -> bool conversion #543) should definitely be kept. The default behavior would be bool(Bool(False)) -> True, which is quite confusing.

[ltalirz]

I'd also like to add that as far as operator overloading goes, a == b on AiiDA types is probably a rather rare use case in plugins (I can't remember ever using it myself). A decision to drop the convenience of overloaded operators/automatic type conversions needs to take into account the most prominent use cases (this may need a bit of searching around in the larger plugins).

[greschd]

My biggest worry with all this is that we're introducing backwards-incompatible changes not for any particularly strong technical reason, but for reasons where reasonable minds (ourselves in the past, and maybe in the future) could come to the opposite conclusion.
Because there's no strong reason for one or the other behavior, it's easy to keep "improving" it while actually going in a circle, and introducing churn downstream while doing so.

That's not to say that I completely oppose this change (as mentioned above), but I think it should be well-considered. Maybe the relationship our (built-in) data nodes have to their Python relatives needs to be noted down in an AEP.

Here we're discussing removing behavior that is present in the Python built-ins from the data node, but in #4351 we're actually going the opposite way.

I'd also warn against the mentality that "we're anyway doing 2.0, let's just change that also" [1]. In my opinion, we should always strive to be as backwards-compatible as possible. Sometimes that isn't possible without compromising the functionality, so then semantic versioning keeps track of that. But each backwards-incompatible change should by itself have a strong enough reason for existing, irrespective of version numbers.
Note that the other backwards-incompatible changes in 2.0 are actually quite minor.

[1] Sorry for the straw-man argument @giovannipizzi - I know that's not what you said above.

[giovannipizzi]

Hi, no worries @greschd - good to hear all opinions!
I fully understand your arguments, and I admit I'm also worried that we start applying changes in circle.

In the end, if we get a reasonable number of people agree that the == behaviour defined by the plugins and falling back to UUIDs is intuitive enough for most people, I'm ok doing it.
More than asking the general question, however, I would however present a few 'usecases' to some selected users and ask what they would expect.

E.g.:

a = Int(1).store()
b = Int(1).store()
a == b # What to you expect?

a = Dict({}).store()
b = Dict({}).store()
a == b # What to you expect?

and so on, including cases where the equality would be true at the moment and cases where it wouldn't.
It will also be useful to decide if instead we need to improve the equality comparison for certain classes (e.g. Dicts, UpfData with the same MD5, ...)

And of course, this needs to be well documented!

[greschd]

Since this came up again in #4736, I think we agree that nodes with the same UUID should compare equal. We could implement that, and punt the question of whether we should also compare by content (by keeping the current behavior). Or does someone want to take up doing that user study, and fix it "properly"?

[ltalirz]

Since this came up again in #4736, I think we agree that nodes with the same UUID should compare equal. We could implement that, and punt the question of whether we should also compare by content (by keeping the current behavior).

Sounds like a good way to make progress. Fully support this.

[giovannipizzi]

In this approach, what should be the behaviour for Int nodes, for instance?

• Int(1) == Int(1) gives true: we go back to the same issue of where to stop (e.g. why dicts do not compare as equal)
• Int(1) == Int(1) gives false: backward incompatible, will break many workflows

I need to re-read the whole thread, but maybe the easiest is to say that == compares at least by UUID, and possibly additionally by value, but two nodes with same "value" could still compare to False if comparison by value is not implemented (and accept that this can change across versions without breaking backward-incompatible: i.e. things start comparing as equal is ok, things that were equal start not being equal anymore is bad).
I hope that I'm not opening a can of worms :-)

Anyway, I still think that it would be great if a volunteer ;-) could come up with a set of examples that we want to then check with users, to get a sense of what people intuitively expect. If we prepare the list here (both which examples to give, and which answers to get: I don't know, e.g. "I expect True, and would be very confused if I get False", "I expect True, but would understand if I get False", "I expect False, and would be very confused if I get True", "I expect False, but would understand if I get True", "I don't know what to expect", ...), we can then just convert it into a Google Forms and send it to the mailing list (maybe after testing it first among developers to see if the questions are well posed).

[greschd]

I'm proposing a simple change to the current behavior: As a last fallback, compare by UUID instead of just returning False. If there is an existing implementation in terms of values, we leave it as is (for now!). Of course, any implementation that compares by value needs to compare the same node as equal if it's to be any use.

I feel the UUID issue is important enough to fix soon (as evidenced by multiple issue cropping up over time), that we can leave the can of worms unopened right now. Probably we should then open a separate issue where we condense the options discussed in this thread.