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 handling of index holes in PyDiGraph pickling #116

Merged
merged 5 commits into from
Sep 21, 2020

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Aug 18, 2020

This commit fixes an issue with pickling (and by extension deepcopy)
PyDiGraph or PyDAG objects that have holes in their node id lists.
Previously the node ids would not be preserved across pickling leading
to a compacted list instead of the original node ids. For example, if
you had a PyDiGraph with node ids [0, 1, 3] after pickling/deepcopy the
node ids would be [0, 1, 2] but otherwise identical. This commit fixes
this issue by adding a check for holes to __setstate__ method and
incrementing the node id to reproduce a 1:1 mapping with the original
node ids prior to pickling.

This commit fixes an issue with pickling (and by extension deepcopy)
PyDiGraph or PyDAG objects that have holes in their node id lists.
Previously the node ids would not be preserved across pickling leading
to a compacted list instead of the original node ids. For example, if
you had a PyDiGraph with node ids [0, 1, 3] after pickling/deepcopy the
node ids would be [0, 1, 2] but otherwise identical. This commit fixes
this issue by adding a check for holes to __setstate__ method and
incrementing the node id to reproduce a 1:1 mapping with the original
node ids prior to pickling.
@mtreinish
Copy link
Member Author

I'm marking this as on hold because this depends on #115 before we can safely remove it. Currently this bug is actually being used by users to workaround #27, so the reordering is a feature. Once there is a fix in place for #27 users will no longer need to run deepcopy() or to pickle objects to compact node indices for doing an isomorphism check. But we don't want to accidentally merge this until that is fixed

@coveralls
Copy link

coveralls commented Aug 18, 2020

Pull Request Test Coverage Report for Build 262946833

  • 37 of 37 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 93.447%

Totals Coverage Status
Change from base Build 261084360: 0.03%
Covered Lines: 2139
Relevant Lines: 2289

💛 - Coveralls

@mtreinish mtreinish removed the on hold label Aug 25, 2020
@mtreinish mtreinish removed the on hold label Sep 19, 2020
@mtreinish mtreinish merged commit 4116d1a into Qiskit:master Sep 21, 2020
@mtreinish mtreinish deleted the fix-deepcopy-index branch September 21, 2020 18:39
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.

2 participants