Bugfix/json preserves node gaps #1517
Conversation
Pull Request Test Coverage Report for Build 21502772359Details
💛 - Coveralls |
|
I will take a look at this during the weekend. Thanks for sending the fix for an issue you found! |
IvanIsCoding
left a comment
IvanIsCoding
left a comment
There was a problem hiding this comment.
Excellent test cases! I have no doubt this works, I just left some comments on what should be improved in Rust.
src/json/node_link_data.rs
Outdated
|
|
||
| id_mapping.insert(node_id, idx); | ||
| // Mark this index as used (remove from tmp_nodes) | ||
| tmp_nodes.retain(|&n| n != idx); |
There was a problem hiding this comment.
This can lead to quadratic behaviour, we are looping through all tmp_nodes to delete the index.
Please use https://docs.rs/hashbrown/latest/hashbrown/struct.HashSet.html instead
tests/graph/test_pickle.py
Outdated
| self.assertEqual(g.node_indices(), gprime.node_indices()) | ||
| self.assertEqual(g.edge_list(), gprime.edge_list()) | ||
| self.assertEqual(g.nodes(), gprime.nodes()) | ||
| self.assertEqual(dict(g.edge_index_map()), dict(gprime.edge_index_map())) |
There was a problem hiding this comment.
Of course you copied examples that don't use it but: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertDictEqual
This just has a nicer error message, in case someone ever introduces a bug
|
Also, you'll need to solve some minor conflicts with #1510 |
3 participants
Addresses #1516: JSON roundtrip is broken whenever the graph contains deleted (or contracted) nodes.
This leaves just the GraphML format as incapable of a clean round-trip.