-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Corona Product #5223
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
Corona Product #5223
Conversation
Todos: Docs Test for corona product
|
Thank you for this PR. The Corona product is a helpful feature to add. And this implementation looks good to me. Can you add Are there performance issues we should think about? I think CPU usage is not an issue. What about memory? If you have a really big H (or G) will there be a bottleneck in new_edges getting huge? If so, you could loop over Thanks! |
|
Hi, Your concern about memory usage in large graphs was correct, so I changed my implementation a little bit to avoid temp lists. Thank you. |
Co-authored-by: Dan Schult <dschult@colgate.edu>
|
oops... my suggestion had quotes around the output string when it shouldn't have. I think taking out those quotes gets rid of the test failures. |
I do not know what to do!!!?! |
|
Take the single quotes out of line 508 where the output of the example appears as |
dschult
left a comment
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.
I have some minor typos in the docs, an extra line to name G and H, and a shorter assert line in the test (which matches the previous test's use of G.size() over G.number_of_edges().
With these changes I'm ready to approve it. Thanks very much!!
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
`size()` is like the `number_of_edges()` method. Co-authored-by: Dan Schult <dschult@colgate.edu>
dschult
left a comment
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.
Thanks!
MridulS
left a comment
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.
Thanks @alifa98!
* Corona product implemented. Todos: Docs Test for corona product * Corona Product Added and Works Correctly now. * document added. * corona product test added * corona product added to __all__ * add corona_product to operators.rst * precommit format fixed * Memory usage problem in the corona product improved * Update networkx/algorithms/operators/product.py Co-authored-by: Dan Schult <dschult@colgate.edu> * temp list replaced with generators * single qoutes problem in doc string * doc clean up Co-authored-by: Dan Schult <dschult@colgate.edu> * doc clean up Co-authored-by: Dan Schult <dschult@colgate.edu> * Update Tests `size()` is like the `number_of_edges()` method. Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Dan Schult <dschult@colgate.edu>
* Corona product implemented. Todos: Docs Test for corona product * Corona Product Added and Works Correctly now. * document added. * corona product test added * corona product added to __all__ * add corona_product to operators.rst * precommit format fixed * Memory usage problem in the corona product improved * Update networkx/algorithms/operators/product.py Co-authored-by: Dan Schult <dschult@colgate.edu> * temp list replaced with generators * single qoutes problem in doc string * doc clean up Co-authored-by: Dan Schult <dschult@colgate.edu> * doc clean up Co-authored-by: Dan Schult <dschult@colgate.edu> * Update Tests `size()` is like the `number_of_edges()` method. Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Dan Schult <dschult@colgate.edu>
Hi all,
I have added the "corona product" of two graphs in the product algorithms.
I also had written a blog post here about this product months ago.
Thank you