Skip to content
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

Fix subgraph handling of non-contiguous nodelist argument #6738

Merged
merged 4 commits into from
Jul 27, 2021

Conversation

mtreinish
Copy link
Member

Summary

This commit fixes the subgraph() method on the coupling map class when
the node list had a non-contiguous list that didn't start at 0.
Previously, the subgraph method would inject nodes for the holes in the
node list so that all the indices specified in the nodelist argument
were present in the output subgraph. But this was neglecting the nodes
get reindexed and wasn't valid. This commit fixes this by just removing
that step and relying on retworkx's subgraph() method to do the
underlying work.

Details and comments

Fixes #6736

@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jul 13, 2021
@mtreinish mtreinish requested a review from ajavadia July 13, 2021 21:06
@mtreinish mtreinish requested a review from a team as a code owner July 13, 2021 21:06
@mtreinish mtreinish changed the title Fix subgraph handling non-contiguous nodelist argument Fix subgraph handling of non-contiguous nodelist argument Jul 13, 2021
This commit fixes the subgraph() method on the coupling map class when
the node list had a non-contiguous list that didn't start at 0.
Previously, the subgraph method would inject nodes for the holes in the
node list so that all the indices specified in the nodelist argument
were present in the output subgraph. But this was neglecting the nodes
get reindexed and wasn't valid. This commit fixes this by just removing
that step and relying on retworkx's subgraph() method to do the
underlying work.

Fixes Qiskit#6736
@ajavadia
Copy link
Member

I just noticed there's a .reduce() method which does basically the same thing (without this bug). Is there a reason to have both?

@mtreinish
Copy link
Member Author

I'm not sure there is a functional difference between them. I didn't even know there was a reduce() method. Looking at the code, reduce() will preserve node list order while subgraph() won't. The subgraph() implementation should be faster, but I don't expect either method to be a bottleneck. We can deprecate subgraph() in a follow up PR if you want, it would be better to merge the fix without deprecation so we can backport it for 0.18.1.

@nonhermitian
Copy link
Contributor

I suspect reduce, or something equivalent, will come in handy in the future to run independent experiments on a single chip simultaneously.

@mergify mergify bot merged commit 0c6890d into Qiskit:main Jul 27, 2021
mergify bot pushed a commit that referenced this pull request Jul 27, 2021
This commit fixes the subgraph() method on the coupling map class when
the node list had a non-contiguous list that didn't start at 0.
Previously, the subgraph method would inject nodes for the holes in the
node list so that all the indices specified in the nodelist argument
were present in the output subgraph. But this was neglecting the nodes
get reindexed and wasn't valid. This commit fixes this by just removing
that step and relying on retworkx's subgraph() method to do the
underlying work.

Fixes #6736

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
(cherry picked from commit 0c6890d)
mergify bot added a commit that referenced this pull request Jul 27, 2021
)

This commit fixes the subgraph() method on the coupling map class when
the node list had a non-contiguous list that didn't start at 0.
Previously, the subgraph method would inject nodes for the holes in the
node list so that all the indices specified in the nodelist argument
were present in the output subgraph. But this was neglecting the nodes
get reindexed and wasn't valid. This commit fixes this by just removing
that step and relying on retworkx's subgraph() method to do the
underlying work.

Fixes #6736

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
(cherry picked from commit 0c6890d)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coupling_map.subgraph() creates extra nodes
4 participants