Added Mesh and Grid graph generator #191
Conversation
Pull Request Test Coverage Report for Build 361925241
💛 - Coveralls |
|
@darknight009 thanks for pushing this. Hmm, I hadn't thought about that, but I'd probably just go with a flattened array. It's consistent with the other generator methods and easier to deal with. We just need to make sure it's documented the order it's used, row major or column major. |
There was a problem hiding this comment.
Thanks for pushing the update. From a quick pass through the code looks good, I left a few initial comments inline mostly around docs cleanups and some edge case handling. But, I haven't dug through each line in depth yet. Besides the docs failure (which I outlined how to fix inline I think). Can you also add an example with the graphviz visualization to the docs? It's kind of nice for the generator documentation to visualize the graph so people can see them in the documentation. Basically something like: https://github.com/Qiskit/retworkx/blob/master/src/generators.rs#L50-L72
src/generators.rs
Outdated
| /// Generate a directed grid graph. The edges propagate towards right and | ||
| /// bottom direction if ``bidirectional`` is ``false`` | ||
| /// | ||
| //// :param int rows: The number of rows to generate the graph with. |
There was a problem hiding this comment.
This should fix the failing docs build I think:
| //// :param int rows: The number of rows to generate the graph with. | |
| /// :param int rows: The number of rows to generate the graph with. |
src/generators.rs
Outdated
| /// If number of nodes(rows*cols) is greater than length of | ||
| /// weights list, extra nodes with None weight are appended | ||
| /// | ||
| /// :returns: The generated star graph |
There was a problem hiding this comment.
| /// :returns: The generated star graph | |
| /// :returns: The generated grid graph |
src/generators.rs
Outdated
| /// :param bidirectional: A parameter to indicate if edges should exist in | ||
| /// both directions between nodes | ||
| /// | ||
| /// :returns: The generated star graph |
There was a problem hiding this comment.
| /// :returns: The generated star graph | |
| /// :returns: The generated grid graph |
There was a problem hiding this comment.
This LGTM, there are 2 inline suggestions that were basically just errors that cargo clippy reported. I should probably add a CI job with it to catch things like this in the future. But after those are fixed this should be ready to go. Thanks for updating the docs.
src/generators.rs
Outdated
| let mut node_cnt = num_nodes; | ||
|
|
||
| for weight in weights { | ||
| if node_cnt <= 0 { |
There was a problem hiding this comment.
I ran cargo clippy on the PR locally and it flagged this. Since node_cnt is a usize it can't ever be less than 0, so they suggested replacing this with:
| if node_cnt <= 0 { | |
| if node_cnt == 0 { |
src/generators.rs
Outdated
| let mut node_cnt = num_nodes; | ||
|
|
||
| for weight in weights { | ||
| if node_cnt <= 0 { |
There was a problem hiding this comment.
I ran cargo clippy on the PR locally and it flagged this. Since node_cnt is a usize it can't ever be less than 0, so they suggested replacing this with:
| if node_cnt <= 0 { | |
| if node_cnt == 0 { |
Fixes Rust Lint error where <0 check was implemented on a usize variable
|
@darknight009 just an fyi thanks to your work on this PR we were able to speed up qiskit-terra's coupling map construction in: Qiskit/qiskit#5469 |
#150 Adds both directed and undirected versions of Mesh and Grid graph generators.
This PR doesn't add test cases for Grid generator as it is unclear how the weights will be passed for the grid. As a multidimensional array or a flattened array?
@mtreinish Please let me know how to proceed with that