Bugfix/json preserves node gaps #1517
Conversation
Pull Request Test Coverage Report for Build 18563991601Details
💛 - 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.
| }, | ||
| None => py.None(), | ||
| }; | ||
| let node_id = node.id.unwrap_or(0); |
There was a problem hiding this comment.
| let node_id = node.id.unwrap_or(0); | |
| let node_id = node.id?; |
I don't think we should default to 0 if it is not defined.
| let mut tmp_nodes: Vec<NodeIndex> = Vec::new(); | ||
| for _ in 0..=max_id { | ||
| let idx = out_graph.add_node(py.None()); | ||
| tmp_nodes.push(idx); | ||
| } |
There was a problem hiding this comment.
Maybe a more idiomatic way of writing this:
let tmp_nodes: Vec<NodeIndex> = (0..=max_id)
.map(|_| out_graph.add_node(py.None()))
.collect();
There was a problem hiding this comment.
This suggestion also works with HashSet by the way
|
|
||
| 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
| } | ||
|
|
||
| has_gaps | ||
| } else { |
There was a problem hiding this comment.
nit picking: can you swap the if/else order? i.e. use if ! preserve_ids.
This is a style choice but I prefer to have the most common case first in if statements.
| 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.