-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add binomial tree graph generator #299
Conversation
Rebasing
Add binomial tree graph for a given order.
Pull Request Test Coverage Report for Build 863756254
💛 - Coveralls |
src/generators.rs
Outdated
if order.is_none() { | ||
return Err(PyIndexError::new_err("order should be specified")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to make this optional if it's always required? Couldn't we change the order argument to be: order: usize,
above and just remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I was thinking to use weights to initialize as well, but was having some issues with rust's Option
.
However, not making order optional would work.
I ran some benchmarks comparing to import time
import retworkx as rx
import networkx as nx
libraries = ["retworkx", "networkx"] # change for each branch
sizes = [5, 10, 20]
start = 0
stop = 0
for lib in libraries:
for N in sizes:
if lib == "retworkx":
start = time.time()
graph = rx.generators.binomial_tree_graph(N)
stop = time.time()
if lib == "networkx":
start = time.time()
graph = nx.generators.binomial_tree(N)
stop = time.time()
print(f"N = {N} {lib}: {str(stop - start)}")
|
This is done because that for `pow`method a u32 is needed.
…orkx into binomial-tree-graph
for n in range(10): | ||
graph = retworkx.generators.binomial_tree_graph(n) | ||
self.assertEqual(len(graph), 2**n) | ||
self.assertEqual(len(graph.edges()), 2**n - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some assertions here on the edge list. It would be good to assert that the output graph is actually a binomial tree instead of just a graph with 2**n
nodes and 2**n - 1
edges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assertions to check if the edges match what is expected for a binomial tree graph
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
…orkx into binomial-tree-graph
`find_edge`makes sure that you don't add repeated edges on the graph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of quick inline comments. I think this is almost ready.
order: u32, | ||
weights: Option<Vec<PyObject>>, | ||
bidirectional: bool, | ||
) -> PyResult<digraph::PyDiGraph> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not do a multigraph flag here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I just did this because all other directed generators have this pattern. Should we add multigraph
on all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize it was missing as an option from all the digraph generators. I think then it's fine to exclude it here for now, but we should push a follow up PR and add a multigraph option to all the digraph generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll raise an issue and submit a PR for that
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I found a couple more places to use is_none()
but other than that I think this is ready.
Related to #150
Add binomial tree graph for a given order.