Fix pickle/deepcopy not preserve original edge indices #589

binh-vu · 2022-04-18T01:16:00Z

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

…iginal edge index

CLAassistant · 2022-04-18T01:16:04Z

All committers have signed the CLA.

coveralls · 2022-04-18T15:52:09Z

Pull Request Test Coverage Report for Build 2182282857

293 of 321 (91.28%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 97.884%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/digraph.rs	147	161	91.3%
src/graph.rs	146	160	91.25%

Totals
Change from base Build 2173032073:	-0.2%
Covered Lines:	11102
Relevant Lines:	11342

💛 - Coveralls

mtreinish

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

mtreinish · 2022-04-21T13:49:36Z

src/digraph.rs

+        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();


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.

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?

src/digraph.rs

dizhouwu · 2023-05-06T15:51:40Z

Any chance this would get merged?

mtreinish · 2023-05-08T19:51:11Z

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.

coveralls · 2023-05-08T21:34:01Z

Pull Request Test Coverage Report for Build 4929430178

250 of 278 (89.93%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 96.936%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/digraph.rs	126	140	90.0%
src/graph.rs	124	138	89.86%

Totals
Change from base Build 4919724815:	-0.1%
Covered Lines:	14519
Relevant Lines:	14978

💛 - 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 <[email protected]>

* 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 <[email protected]> * 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 <[email protected]> * Turn off CI for forks (#868) Co-authored-by: Matthew Treinish <[email protected]> * 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 <[email protected]> * 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <[email protected]> * 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 <[email protected]> * 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] <[email protected]> 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] <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Binh Vu <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: matanco64 <[email protected]>

dizhouwu · 2023-05-14T20:15:40Z

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 <[email protected]> * 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 <[email protected]> * Turn off CI for forks (Qiskit#868) Co-authored-by: Matthew Treinish <[email protected]> * 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 <[email protected]> * 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <[email protected]> * 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 <[email protected]> * 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] <[email protected]> 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] <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Binh Vu <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: matanco64 <[email protected]>

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

fix issue Qiskit#585 that pickling graph & digraph do not preserve or…

f3bc9f3

…iginal edge index

fix clippy lints - collapsible_else_if

49986bf

mtreinish reviewed Apr 21, 2022

View reviewed changes

mtreinish added 2 commits May 8, 2023 16:36

Merge remote-tracking branch 'origin/main' into binh-vu-main

f3b0231

Merge branch 'main' into main

e031b39

mtreinish added 2 commits May 9, 2023 13:27

Simplify logic in __setstate__

73e6594

Add release note

84c365e

mtreinish changed the title ~~fix issue #585 that pickling graph & digraph do not preserve original edge index~~ Fix pickle/deepcopy not preserve original edge indices May 9, 2023

mtreinish added 2 commits May 9, 2023 14:34

Merge branch 'main' into main

2125fa2

Fix lint

777fec2

mtreinish approved these changes May 9, 2023

View reviewed changes

mtreinish merged commit 8686896 into Qiskit:main May 9, 2023

mtreinish mentioned this pull request Jun 5, 2023

Fix pickle/deepcopy node hole handling #888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pickle/deepcopy not preserve original edge indices #589

Fix pickle/deepcopy not preserve original edge indices #589

binh-vu commented Apr 18, 2022 •

edited by mtreinish

Loading

CLAassistant commented Apr 18, 2022 •

edited

Loading

coveralls commented Apr 18, 2022

mtreinish left a comment

mtreinish Apr 21, 2022

binh-vu Apr 21, 2022

dizhouwu commented May 6, 2023

mtreinish commented May 8, 2023

coveralls commented May 8, 2023 •

edited

Loading

dizhouwu commented May 14, 2023

Fix pickle/deepcopy not preserve original edge indices #589

Fix pickle/deepcopy not preserve original edge indices #589

Conversation

binh-vu commented Apr 18, 2022 • edited by mtreinish Loading

CLAassistant commented Apr 18, 2022 • edited Loading

coveralls commented Apr 18, 2022

Pull Request Test Coverage Report for Build 2182282857

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Apr 21, 2022

Choose a reason for hiding this comment

binh-vu Apr 21, 2022

Choose a reason for hiding this comment

dizhouwu commented May 6, 2023

mtreinish commented May 8, 2023

coveralls commented May 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4929430178

💛 - Coveralls

dizhouwu commented May 14, 2023

binh-vu commented Apr 18, 2022 •

edited by mtreinish

Loading

CLAassistant commented Apr 18, 2022 •

edited

Loading

coveralls commented May 8, 2023 •

edited

Loading