Fixed the endpoint comparison bug #109
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updated files:
causallearn/graph/Endpoint.py: reload__eq__.causallearn/graph/Edge.py;Edges.pyandcausallearn/utils/GraphUtils.py: replace all the "is Endpoint" with "== Endpoint", since identity comparison is unsafe.Reproduce the issue (as of 446b9a2):
According to Python doc, equality comparisons for
Enumare defined by identity comparison (i.e.,is). Here we need to explicitly reimplement__eq__for attributed value comparison. See stackoverflow 1 2.Incorrect SHD caused:
I experienced an incorrect SHD result due to this bug. The truth contains
X1 --> X2and the estimation containsX2 --> X1(given by GES), but the returned SHD is 0, because this line is evaluated as False (i.e., ARROW != ARROW).Potentially there might be more bugs related:
Might be a fatal bug:
Endpoint comparisons are in the basic building blocks, while they're not used uniformly:
causal-learn/causallearn/graph/Edges.py
Line 37 in 446b9a2
causal-learn/causallearn/graph/SHD.py
Line 44 in 446b9a2
causal-learn/causallearn/graph/Edges.py
Line 59 in 446b9a2
causal-learn/causallearn/graph/Edge.py
Line 84 in 446b9a2
Let's take care of this if there will be a graph class refactoring.
Merge plan
I'm not sure whether this pr should be merged right away. On the one hand, we need to ensure that the users receive accurate results (especially SHDs for evaluation in papers). On the other hand, we need to regenerate all the test benchmarks (which takes time) so that all the tests can pass. What do you think? @kunwuz @tofuwen