Adding Generalized Petersen Graph #8147
Conversation
dschult
left a comment
dschult
left a comment
There was a problem hiding this comment.
Thanks for this. I have some comments below.
|
I have updated this Generalized Petersen Graph pull request. I added tests in the :) Should be ready for review. |
| raise NetworkXError(f" Got {n=} {k=}. Need 1 <= k <= n/2") | ||
|
|
||
| G = nx.cycle_graph(range(n), create_using=create_using) # u-nodes | ||
| if G.is_directed(): |
There was a problem hiding this comment.
Is multi graph possible? I think there is only one class that makes sense here. Should we drop create_using?
There was a problem hiding this comment.
We keep the create_using so that folks can pass in their own duck-type-compatible graph classes if they want to. It looks like an antipattern in NX itself, but it's a nice feature for downstream folks!
There was a problem hiding this comment.
We also allow a multigraph in all the small.py generators because the result is the desired graph even if it does not have minimal possible graph class representation. I mean the nodes and edges are the same for both Graph and MultiGraph versions. So petersen_graph(create_using=nx.MultiGraph) is the Petersen Graph -- it's just a multigraph representation without any multiedges. It doesn't contain any multiedges. But it could be changed to contain multiedges. Anyway, someone might want to do that -- and it is correct. So we allow MultiGraph (and any undirected subclass of Graph that a user creates).
rossbar
left a comment
rossbar
left a comment
There was a problem hiding this comment.
LGTM, thanks @rudyarthur and @dschult , and @amcandio for the insightful comments!
| assert [d for n, d in G.degree()] == 46 * [3] | ||
|
|
||
| # Test create_using with directed or multigraphs on small graphs | ||
| pytest.raises(nx.NetworkXError, nx.tutte_graph, create_using=nx.DiGraph) |
There was a problem hiding this comment.
Do we want to add this one back below? Or perhaps it's duplicated elsewhere and I just missed it since I'm looking only at the diff... if so, ignore!
There was a problem hiding this comment.
This test was duplicated in the next test function "tests_raises_with_directed_create_using" so I removed it here. Thanks for noticing that and asking -- I should have mentioned it in the PR description.
|
LGTM too! |
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
4 participants
This PR adds Generalized Petersen Graph as discussed in #8133
@dschult is going to take this on and hopefully get it merged.