Skip to content
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 Tadpole graph #6999

Merged
merged 15 commits into from
Oct 23, 2023
Merged

Add Tadpole graph #6999

merged 15 commits into from
Oct 23, 2023

Conversation

ghost
Copy link

@ghost ghost commented Oct 9, 2023

Implements the Tadpole graph
Source: https://en.wikipedia.org/wiki/Tadpole_graph

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't hard to construct from the others, but it does seem to be referred to regularly in the literature and might be common enough to warrant inclusion. Some comments appear below.

networkx/generators/classic.py Outdated Show resolved Hide resolved
networkx/generators/classic.py Show resolved Hide resolved
networkx/generators/classic.py Show resolved Hide resolved
peijenburg and others added 4 commits October 11, 2023 09:46
Co-authored-by: Dan Schult <dschult@colgate.edu>
@ghost ghost requested a review from dschult October 11, 2023 08:38
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a question as to whether we should allow a tadpole to have m=0 (no head to the tadpole). I'm +0 for allowing that. It differs from the treatment for lollipop_graph where the head must contain at least 2 nodes (to be a complete graph). Here we have a cycle connected to the path, and a cycle can have a single node (with a self-loop) or no nodes.

I have a few suggestions below and otherwise think this is ready to merge.
Thanks!

networkx/generators/tests/test_classic.py Outdated Show resolved Hide resolved
networkx/generators/classic.py Show resolved Hide resolved
networkx/generators/classic.py Outdated Show resolved Hide resolved
networkx/generators/classic.py Outdated Show resolved Hide resolved
networkx/generators/tests/test_classic.py Outdated Show resolved Hide resolved
peijenburg and others added 4 commits October 23, 2023 09:19
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
@ghost
Copy link
Author

ghost commented Oct 23, 2023

I probably clicked a wrong button, because I cannot see my own reply, but here's my opinion;

Using m=1 should, IMO, give a single node without links. @dschult suggested a single node with a self-loop. One possible way to avoid these ambiguities is to enforce m >= 2. But my preference would be to have m=1 and general n to return a path graph with n+1 nodes.

I also don't understand what's wrong with the CI

@ghost
Copy link
Author

ghost commented Oct 23, 2023

Changed to throw an error when m<2. Still cannot find out why CI fails.

@ghost ghost requested a review from dschult October 23, 2023 15:23
@dschult
Copy link
Member

dschult commented Oct 23, 2023

To fix CI, merge the upstream main with your branch.
Steps are hopefully something like:

git remote add upstream https://github.com/networkx/networkx
git fetch upstream main
git ch Add-tadpole-graph   # if needed
git pull upstream main --rebase

The last line will try to apply your commits since you diverged from main on top of the current main branch.
If you have changed a line that was also changed on main you will get a "conflict" and you'll have to follow the instructions to edit the file by hand to specify which changes you want to keep. That can be a mess sometimes.
Can you try it and see if everything "just works"? If it doesn't, I can work through that. Thanks.

The last line can also be: git merge upstream main which I believe is the same as git pull upstream main --no-rebase and that can sometimes be easier to deal with the conflicts. But it adds an extra commit if I remember the details right.

I think the conflicts are also on another of your PRs. I have on my list to work through those conflicts. :}

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ready to me!

@jarrodmillman jarrodmillman merged commit 8311f1b into networkx:main Oct 23, 2023
35 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Oct 23, 2023
@ghost ghost deleted the Add-tadpole-graph branch October 24, 2023 09:21
Alex-Markham pushed a commit to Alex-Markham/networkx that referenced this pull request Oct 26, 2023
* Add Tadpole graph

* style fix

* doc update

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update classic.py

* apply suggestions

* style fix

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* style fix

* error on m<2

* style fix

* simplify

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Massimo Achterberg <maachterberg@tudelft.nl>
@jarrodmillman jarrodmillman mentioned this pull request Oct 27, 2023
jarrodmillman pushed a commit that referenced this pull request Oct 27, 2023
* Add Tadpole graph

* style fix

* doc update

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update classic.py

* apply suggestions

* style fix

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* style fix

* error on m<2

* style fix

* simplify

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Massimo Achterberg <maachterberg@tudelft.nl>
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Add Tadpole graph

* style fix

* doc update

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update classic.py

* apply suggestions

* style fix

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Update networkx/generators/tests/test_classic.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* style fix

* error on m<2

* style fix

* simplify

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Massimo Achterberg <maachterberg@tudelft.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants