Add functions to parse node link JSON #1091
Conversation
This commit adds the missing functionality to parse node link json and generate rustworkx graph objects from it. This adds two new functions parse_node_link_json_str() to parse a node link json string and parse_node_link_json_file() to parse a node link json file from a path. Partial Qiskit#840
Pull Request Test Coverage Report for Build 7937155302Details
💛 - Coveralls |
There was a problem hiding this comment.
I think this is a great addition. The code is very succint and I think we leverage serde well.
My main concern is with the function naming. Maybe we could have rustworkx.parse_node_link_json for the string case and rustworkx.node_link_json_from_file for the file case? I just found parse_node_link_json_str a name that leaves a bad taste in the mouth
I also left a comment about the NetworkX round-trip test. If the test passes, I wonder how fast it will be compared to the current NetworkX.
| rustworkx.parse_node_link_json_file | ||
| rustworkx.parse_node_link_json_str |
There was a problem hiding this comment.
I really dislike the name of the functions. Maybe we can come up with alternatives? They feel ugly
There was a problem hiding this comment.
Yeah, I wasn't happy with them either tbh. I'll switch them to your suggested parse_node_link_json and node_link_json_from_file those are better names.
| self.assertEqual(new.weighted_edge_list(), graph.weighted_edge_list()) | ||
| self.assertEqual(new.attrs, graph.attrs) | ||
|
|
||
| def test_round_trip_with_file(self): |
There was a problem hiding this comment.
Overall this is super cool! Do you think we should add a round trip (optional) test where we write from NetworkX and them import it in Rustworkx?
There was a problem hiding this comment.
Sure, that's simple enough to add.
There was a problem hiding this comment.
This was a good call to add a test for this. The parser is overly strict because it's assuming there was always an id field for edges which networkx doesn't always populate in it's node link json. This isn't required in the format, so I'll tweak the parser to make it optional.
This commit adds the missing functionality to parse node link json and generate rustworkx graph objects from it. This adds two new functions parse_node_link_json_str() to parse a node link json string and parse_node_link_json_file() to parse a node link json file from a path.
Partial #840