Skip to content

Comments

Fix the __eq__ method of GeneralGraph and add __hash__#254

Merged
kunwuz merged 5 commits intopy-why:mainfrom
Ykabrit:fix/251-general-graph-eq-hash
Feb 18, 2026
Merged

Fix the __eq__ method of GeneralGraph and add __hash__#254
kunwuz merged 5 commits intopy-why:mainfrom
Ykabrit:fix/251-general-graph-eq-hash

Conversation

@Ykabrit
Copy link
Contributor

@Ykabrit Ykabrit commented Feb 10, 2026

This PR resolves #251 by modifying the __eq__ method in GeneralGraph.
As implemented initially, this method breaks graphs by sorting self.nodes in-place (which renders the list of nodes incoherent with self.graph and self.node_map), as well as returning False when two identical (but having different node ordering in self.nodes) graphs because it compares self.graph and other.graph which are not identical if the node ordering was different.

In the modified method, self.nodes and other.nodes are both sorted using the sorted() method and stored in local variables. Secondly, rows and columns of other.graph are permutated in order to correspond to self.graph and make them comparable, which was not the case before.

Not entirely related, but I also added __hash__ methods to GeneralGraph and Edge.

Ykabrit and others added 4 commits February 10, 2026 14:34
Signed-off-by: Ykabrit <ykabrit@yanibou.fr>
Signed-off-by: Ykabrit <ykabrit@yanibou.fr>
Signed-off-by: Ykabrit <ykabrit@yanibou.fr>
Copy link
Collaborator

@zhi-yi-huang zhi-yi-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I have one suggestion regarding the hash calculation: instead of computing hash(node_tuple) + hash(graph_tuple) and using that sum as the combined hash, it would be better to combine the two tuples into a single tuple first and then hash that—i.e., hash((node_tuple, graph_tuple)). This helps to better maintain the uniformity and uniqueness of hashes.

Signed-off-by: Ykabrit <ykabrit@yanibou.fr>
@Ykabrit
Copy link
Contributor Author

Ykabrit commented Feb 17, 2026

You're right! Here, normally it's fixed.

@Ykabrit Ykabrit requested a review from zhi-yi-huang February 17, 2026 12:23
Copy link
Collaborator

@zhi-yi-huang zhi-yi-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kunwuz kunwuz merged commit f66d0f9 into py-why:main Feb 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error in the __eq__() method of the GeneralGraph class

3 participants