‼️ BREAKING: Compare Dict nodes by content
#5251
Conversation
Currently there is an inconsistency in how the base data type node instances compare equality. All base types compare based on the content of the node, whereas `Dict` instances rely on the UUID fallback introduced in aiidateam#4753. After a long discussion started by aiidateam#1917, it was finally decided that the best way forward is to make the equality comparison consitent among the base types (see aiidateam#5187). Here we adapt the `__eq__` method of the `Dict` class to compare equality by content instead of relying on the fallback comparison of the UUIDs.
Codecov Report
@@ Coverage Diff @@
## develop #5251 +/- ##
===========================================
- Coverage 81.39% 81.38% -0.00%
===========================================
Files 529 529
Lines 37009 37002 -7
===========================================
- Hits 30119 30112 -7
Misses 6890 6890
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks @muhrin! I'll leave the merging to coding week merge master @chrisjsewell. |
There was a problem hiding this comment.
Thanks @mbercx . See comment of removing __ne__. Would do same for other base types that have just plain negation of __eq__. Then I think this OK to merge. Final important thing would be to update the wiki page for v2.0 migration and make a note of this important change.
aiida/orm/nodes/data/dict.py
Outdated
| def __ne__(self, other): | ||
| return not self == other |
There was a problem hiding this comment.
I think we should remove this definition of __ne__. By default __ne__ is the negation of __eq__ as stated in the official documentation and so it is recommended not to implement it unless it needs specific functionality
There was a problem hiding this comment.
Thanks @sphuber! Wasn't aware this was the default behaviour, will remove it for List and BaseType as well then.
|
Thanks for the review @sphuber! I've removed the |
|
Yes, I would say so. It might not be the last time we discuss how to compare node equality, but I think we've all settled on "base types compare content all others compare UUID by default" for now. I'll update the OP.
Not afaik. |
Fixes #1917
Currently there is an inconsistency in how the base data type node
instances compare equality. All base types compare based on the content
of the node, whereas
Dictinstances rely on the UUID fallbackintroduced in #4753. After a long discussion started by #1917, it was
finally decided that the best way forward is to make the equality
comparison consitent among the base types (see #5187).
Here we adapt the
__eq__method of theDictclass to compareequality by content instead of relying on the fallback comparison of
the UUIDs.