-
Notifications
You must be signed in to change notification settings - Fork 145
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 pickle/deepcopy not preserve original edge indices #589
Conversation
…iginal edge index
Pull Request Test Coverage Report for Build 2182282857
💛 - Coveralls |
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.
Thanks for pushing this fix, overall it LGTM. I have a few inline comments most are just idle thoughts or questions. I think the only real blocker is maintaining support with the old pickle format.
Also while you're updating the branch would you mind adding a fix release note. You can see the process for this here: https://github.com/Qiskit/retworkx/blob/main/CONTRIBUTING.md#release-notes
src/digraph.rs
Outdated
if !self.node_removed { | ||
for item in nodes_lst.iter() { | ||
let node_w = item | ||
.downcast::<PyTuple>() | ||
.unwrap() | ||
.get_item(1) | ||
.unwrap() | ||
.extract() | ||
.unwrap(); | ||
self.graph.add_node(node_w); | ||
} | ||
} else if nodes_lst.len() == 1 { | ||
// graph has only one node, handle logic here to save one if in the loop later | ||
let item = nodes_lst | ||
.get_item(0) | ||
.unwrap() | ||
.downcast::<PyTuple>() | ||
.unwrap(); | ||
let node_idx: usize = item.get_item(0).unwrap().extract().unwrap(); | ||
let node_w = item.get_item(1).unwrap().extract().unwrap(); | ||
|
||
for _i in 0..node_idx { | ||
self.graph.add_node(py.None()); | ||
} | ||
self.graph.add_node(node_w); | ||
for i in 0..node_idx { | ||
self.graph.remove_node(NodeIndex::new(i)); | ||
} | ||
} else { | ||
let last_item = nodes_lst | ||
.get_item(nodes_lst.len() - 1) | ||
.unwrap() | ||
.downcast::<PyTuple>() | ||
.unwrap(); | ||
|
||
// use a pointer to iter the node list | ||
let mut pointer = 0; | ||
let mut next_node_idx: usize = nodes_lst | ||
.get_item(pointer) | ||
.unwrap() | ||
.downcast::<PyTuple>() | ||
.unwrap() | ||
.get_item(0) | ||
.unwrap() | ||
.downcast::<PyLong>() | ||
.unwrap() | ||
.extract() | ||
.unwrap(); | ||
|
||
// list of temporary nodes that will be removed later to re-create holes | ||
let node_bound_1: usize = last_item.get_item(0).unwrap().extract().unwrap(); | ||
let mut tmp_nodes: Vec<NodeIndex> = | ||
Vec::with_capacity(node_bound_1 + 1 - nodes_lst.len()); | ||
|
||
let second_last_node_idx: usize = nodes_lst | ||
.get_item(nodes_lst.len() - 2) | ||
.unwrap() | ||
.downcast::<PyTuple>() | ||
.unwrap() | ||
.get_item(0) | ||
.unwrap() | ||
.downcast::<PyLong>() | ||
.unwrap() | ||
.extract() | ||
.unwrap(); |
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 it might be a bit easier to read if we created an intermediate Vec<(usize, PyObject)>
here. It will eat a bit more memory since we're storing things twice but I'm not sure that's a big concern since we're just doubling the number of node indices in memory as the python data is just a reference. Just a thought, this works fine and we don't really need to change it, it's just a long chain of unwraps and extracts.
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 don't like that too. What I was thinking is to create inline functions for get_index
, and get_value
. But I don't know where to put it. May be at the end of the file?
Any chance this would get merged? |
There were some issues I outlined in review that I thought needed to be updated before we merged. But it looks like the has stalled since my initial review. I'll take a quick look to see if I can rebase it and then fix the issues I outlined. |
Pull Request Test Coverage Report for Build 4929430178
💛 - Coveralls |
* fix issue Qiskit#585 that pickling graph & digraph do not preserve original edge index * fix clippy lints - collapsible_else_if * Simplify logic in __setstate__ * Add release note * Fix lint --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
* Add tests from the example * Fix bug * Fix tests * Add release note * Update release note * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Fix docs to work with Sphinx Theme 1.11 (#867) * Fix docs to work with Sphinx Theme 1.11 * Update docs/source/_templates/sidebar.html Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Turn off CI for forks (#868) Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Fix pickle/deepcopy not preserve original edge indices (#589) * fix issue #585 that pickling graph & digraph do not preserve original edge index * fix clippy lints - collapsible_else_if * Simplify logic in __setstate__ * Add release note * Fix lint --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Bump serde from 1.0.160 to 1.0.162 (#863) Bumps [serde](https://github.com/serde-rs/serde) from 1.0.160 to 1.0.162. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.160...1.0.162) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add reverse inplace function for digraph (#853) * added a reverse_inplace function in digraph, the function reverses the direction of the edges in the digraph implemented by switching the indices of the nodes in an edge. * added python tests for the reverse_inplace function. testing a simple case and a case for a large graph. * ran rust fmt and clippy, also added more detailed documentation * rename reverse_inplace to reverse * change excepts to unwraps (If this fails is because of PyO3. It panics and there is not much point in printing a message) * added tests for empty graph and graph with node removed in the middle * added interface signature for IDEs * ran cargo fmt * Fix doc syntax --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Bump serde from 1.0.162 to 1.0.163 (#869) Bumps [serde](https://github.com/serde-rs/serde) from 1.0.162 to 1.0.163. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.162...v1.0.163) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Extend fixes to add_edges_from and add_edges_from_no_data * Lower amount of nodes in test --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Co-authored-by: Binh Vu <binh-vu@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: matanco64 <38103422+matanco64@users.noreply.github.com>
Thank you so much for merging this - wondering if we could be getting a release soon? |
* Add tests from the example * Fix bug * Fix tests * Add release note * Update release note * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Fix docs to work with Sphinx Theme 1.11 (Qiskit#867) * Fix docs to work with Sphinx Theme 1.11 * Update docs/source/_templates/sidebar.html Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Turn off CI for forks (Qiskit#868) Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Fix pickle/deepcopy not preserve original edge indices (Qiskit#589) * fix issue Qiskit#585 that pickling graph & digraph do not preserve original edge index * fix clippy lints - collapsible_else_if * Simplify logic in __setstate__ * Add release note * Fix lint --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Bump serde from 1.0.160 to 1.0.162 (Qiskit#863) Bumps [serde](https://github.com/serde-rs/serde) from 1.0.160 to 1.0.162. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.160...1.0.162) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Add reverse inplace function for digraph (Qiskit#853) * added a reverse_inplace function in digraph, the function reverses the direction of the edges in the digraph implemented by switching the indices of the nodes in an edge. * added python tests for the reverse_inplace function. testing a simple case and a case for a large graph. * ran rust fmt and clippy, also added more detailed documentation * rename reverse_inplace to reverse * change excepts to unwraps (If this fails is because of PyO3. It panics and there is not much point in printing a message) * added tests for empty graph and graph with node removed in the middle * added interface signature for IDEs * ran cargo fmt * Fix doc syntax --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Bump serde from 1.0.162 to 1.0.163 (Qiskit#869) Bumps [serde](https://github.com/serde-rs/serde) from 1.0.162 to 1.0.163. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.162...v1.0.163) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Extend fixes to add_edges_from and add_edges_from_no_data * Lower amount of nodes in test --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Co-authored-by: Binh Vu <binh-vu@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: matanco64 <38103422+matanco64@users.noreply.github.com>
This commit fixes an issue introduced by Qiskit#589 where in certain cases node holes in a graph would result in a panic being raised. This was caused by a logic bug in trying to recreate the holes. Additionally, there were several places where graph methods removed nodes that the flag to indicate there were removals would no be set. This commit fixes all of these issues so that deepcopy/pickle works as expected.
* Fix pickle/deepcopy node hole handling This commit fixes an issue introduced by #589 where in certain cases node holes in a graph would result in a panic being raised. This was caused by a logic bug in trying to recreate the holes. Additionally, there were several places where graph methods removed nodes that the flag to indicate there were removals would no be set. This commit fixes all of these issues so that deepcopy/pickle works as expected. * Fix test failures * Fix lint * Update src/digraph.rs
This PR fixes issue #585. Now pickling graph & digraph should keep the original edge index.
Did a simple benchmark with
directed_mesh_graph(300)
(no node removed and 50 nodes removed -- 1/6 of the graph). Performance is very similar to previous implementation. If we do not need to maintain the original edge index, the new code is ~10% faster. The serialized data size of the graph is almost the same as previous implementation if no edge is removed. When 1/6 of the graph is removed, the serialized size is increased by about 6%.Fixes #585