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 an initial parameter to simple and dual Barábasi-Albert random graphs #4659

Merged
merged 12 commits into from
May 20, 2021

Conversation

Aufinal
Copy link
Contributor

@Aufinal Aufinal commented Mar 6, 2021

As asked in #3459, adds a new initial parameter to Barábasi-Albert random graph models that allows the user to specify a base graph.

Notes:

  • my IDE formatted some imports automatically; I left them in the PR but I can remove those changes if needed,
  • as noted in this paper (page 3), and somewhat in barabasi_albert_graph does not start off with a connected network #3281, the BA model is ill-defined whenever some degrees are zero. For this reason, the initial graphs for barabasi_albert_graph and dual_barabasi_albert_graph have been changed to stars on (m+1) and max(m1, m2) + 1 vertices, respectively. For the simple model, this does not change the end result, since the first added vertex was always connected to the m others.
  • The tests have been improved: previously, the repeat parameter was useless since the seed (and hence the randomness) was fixed. The new behavior tests the functions for several distinct seeds.

Fixes #3459, fixes #3281

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 great -- and is overdue -- so Thank You!
@hagberg might want to look too.

My pass through the code didn't take long -- well done -- and I only came up with a couple of questions -- some of which aren't really related to the heart of the PR at all. I don't want them to bog down getting this merged but I will put them here anyway and if we get off track we can move that discussion to another PR.

  • Should the keyword argument be more descriptive at the expense of length? initial to initial_graph? I'm not happy with either. Can anyone find a better name? initial is actually pretty good.
  • Why do we import all these functions at the top of the test module? They are all at the top level of the nx namespace. The list at the top does tell us which functions are tested in the module. And putting nx. in front of each function will take some work. But the current treatment is not like our other test modules. What is the current best practice?

networkx/generators/tests/test_random_graphs.py Outdated Show resolved Hide resolved
@Aufinal
Copy link
Contributor Author

Aufinal commented Mar 7, 2021

  • Regarding the keyword argument, initial does seem like the most natural to me, although I'm completely open to changing it.
  • I admit that introducing the toplevel networkx import introduced some redundancy, but I didn't want to clutter this PR by making those changes. As an aside, being a new contributor, I couldn't find any best practices on how to write tests... Maybe this causes some of the clutter/confusion in the tests ?

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.

I'm still reluctant to use initial here -- especially because I like this idiom and expect it to get used elsewhere. Does anyone have a preference between initial_graph and starting_from?

These comments are picking nits. If you don't have time, let me know.
I think this is very close to being merged.

networkx/generators/random_graphs.py Outdated Show resolved Hide resolved
networkx/generators/random_graphs.py Outdated Show resolved Hide resolved
networkx/generators/random_graphs.py Outdated Show resolved Hide resolved
networkx/generators/random_graphs.py Outdated Show resolved Hide resolved
networkx/generators/random_graphs.py Show resolved Hide resolved
networkx/generators/random_graphs.py Show resolved Hide resolved
@dschult dschult added this to the networkx-2.6 milestone Apr 25, 2021
Co-authored-by: Dan Schult <dschult@colgate.edu>
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.

I think this is ready to merge

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @Aufinal !

@rossbar rossbar merged commit c5c8bfc into networkx:main May 20, 2021
MridulS pushed a commit to MridulS/networkx that referenced this pull request Feb 4, 2023
…om graphs (networkx#4659)

Adds an initial_graph parameter to allow users to specify a base graph to
the BA and dual BA genertors.

Co-authored-by: Ludovic Stephan <ludovic.stephan@ens.fr>
Co-authored-by: Dan Schult <dschult@colgate.edu>
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
…om graphs (networkx#4659)

Adds an initial_graph parameter to allow users to specify a base graph to
the BA and dual BA genertors.

Co-authored-by: Ludovic Stephan <ludovic.stephan@ens.fr>
Co-authored-by: Dan Schult <dschult@colgate.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants