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

Graph instance modified in place by scale_free_graph #4729

Closed
rossbar opened this issue Apr 7, 2021 · 4 comments · Fixed by #5697
Closed

Graph instance modified in place by scale_free_graph #4729

rossbar opened this issue Apr 7, 2021 · 4 comments · Fixed by #5697

Comments

@rossbar
Copy link
Contributor

rossbar commented Apr 7, 2021

nx.scale_free_graph has a create_usingargument which accepts different types of inputs. When a graph instance is provided, it is used as a starting point for the graph to which more nodes will be added as is clearly described in the docstring.

However, the instance passed in via create_using is also modified in place:

>>> G = nx.scale_free_graph(10)
>>> G.nodes()
NodeView((0, 1, 2, 3, 4, 5, 6, 7, 8, 9))
>>> H = nx.scale_free_graph(20, create_using=G)
>>> G.nodes()
NodeView((0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19))

This behavior should either be documented, or the create_using argument should be copied (when a graph instance) to prevent the in-place modification. I think the second option is more consistent with the other functions in NetworkX, which generally try to avoid in-place modification of inputs.

@dschult
Copy link
Member

dschult commented Apr 7, 2021

create_using is a strange kind of beast (see NXEP3). I thought we never used it as a starting graph from which to add nodes and edges. Though apparently scale_free_graph does that -- (see below). Learn something every day. :}

The standard treatment is that create_using, if it is a NetworkX Graph, is cleared before using it to create the graph. If it is a graph constructor (like nx.Graph) then it is called to create the starting graph.

So... That argument -- create_using is always modified when it is an instance. Maybe NXEP3 can end up fixing that behavior too.

@rossbar
Copy link
Contributor Author

rossbar commented Apr 7, 2021

The standard treatment is that create_using, if it is a NetworkX Graph, is cleared before using it to create the graph

Yes, this is the first instance that I've come across where this wasn't the case. In this particular case, I think this may just be the result of imprecise argument naming. create_using doesn't really make sense in this context because the graph must be a MultiDiGraph, so the parameter is really only for passing in an initial graph instance to build upon, not selecting a graph type.

I think the intended behavior would be better reflected by a better argument name, e.g. initial_graph with behavior like:

if initial_graph is not None:
    G = initial_graph.copy()
else:
    G = nx.MultiDiGraph([(0, 1), (1, 2), (2, 0)])  # This is the default initialization for the current implementation

@dschult
Copy link
Member

dschult commented Apr 28, 2021

The feature of having a graph to start building a new graph also appears in #4659 and we should probably be consistent in our choice of names. Should the argument be named initial_graph or seed_graph?

@rossbar
Copy link
Contributor Author

rossbar commented May 2, 2021

Should the argument be named initial_graph or seed_graph?

I think both names are good and equally clear. I have a slight preference for initial_graph only because the term "seed" is so commonly associated with random number generation, but this is very much a nit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants