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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing CouplingMap.__eq__ #9766
Implementing CouplingMap.__eq__ #9766
Changes from 5 commits
1a7ea7b
80a79ea
c171a20
fe7165e
ffe2454
ed6b391
f7797ea
4cc84ec
f1c84ca
507eb31
66ad0ab
8bbda72
d1f399e
9c5886a
d89a826
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I was thinking about overnight is whether this is actually the behavior we want on
__eq__
or not. An isomorphic graph is structurally equivalent, but that might not be what people expect with equals. I know For example, the case I was thinking of was if you had two coupling maps:with this PR
cmap_a == cmap_b
would beTrue
, but would the normal expectation be for that to beFalse
?I'm wondering if changing this to be a comparison of the edge lists instead of isomorphism would be a better
__eq__
definition. If so we can add anequiv()
orisomorphic()
method to capture this use case too.I know that I was the one who suggested this approach for
__eq__
originally, but do you have any thoughts here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this - I think the expectation is that
__eq__
should be quite strict. Especially because theCouplingMap
implicitly relates physical qubits to their connections - if I havecm1 == cm2
andcm1
has an edge between physical qubits 0 and 1, I'd expect to be able to interact physical qubits 0 and 1 on the machine backed bycm2
too, but if we relax__eq__
to isomorphism, that won't be the case.imo, especially from my pain dealing with other places in Qiskit where this isn't the case,
__eq__
should be a fairly strict but cheap method wherever possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the point that node labelling matters here. @mtreinish you're suggesting edge list comparison because it uses the actual labels? I guess simplest thing would be to convert edge lists to sets and check equality of sets. Are there any ordering guarantees on the edge lists, or is there no way around something like the set comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I guess:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the edge lists being equal sufficient? Could there be disconnected nodes to worry about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a good point, in it's current form an edge list comparison is fine because the graph needs to be connected. But after #9710 this won't be the case and there can be disconnected nodes in the graph. But I assume this will merge before #9710 so lets assume we don't have to worry about disconnected nodes for this PR and I'll update
__eq__
in #9710 after this merges.As for the edge list order I think you're correct that wrapping it in a
set
is probably more robust to ignore edge order. The returned edges inPyDiGraph.edge_list()
are always in insertion/creation order, but that could differ between graphs that I think should should be==
. So we should do the comparison in an order agnostic way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe having a comment here to say this will create a 5 node coupling map with 3 disconnected (it might not be obvious from a quick glance).