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

networkx.MultiDiGraph.remove_edges_from has inconsistent documentation (v2.6.2) #5158

Closed
mikidep opened this issue Oct 28, 2021 · 4 comments
Closed

Comments

@mikidep
Copy link

mikidep commented Oct 28, 2021

In this doc page the parameters list reads that if the elements of ebunch are

2-tuples (u, v) All edges between u and v are removed.

However, both in the second example below and in my tests, it seems that only one edge between u and v is removed. The functionality described in the parameters list would be useful to have.

@dschult
Copy link
Member

dschult commented Oct 28, 2021

This behavior was changed in in remove_edge back in #284 and it looks like the docs in remove_edges_from never got updated to reflect this change. It is not clear that the intent was to change remove_edges_from.... just the remove_edge method.

A workaround way to remove all edges from a MultiDiGraph between nodes u and v is:

DiGraph.remove_edge(self, u, v)

The longer term fix is to

  1. change the documentation to match what remove_edges_from actually does for multigraphs.
  2. decide whether to change the current behavior (backward incompatible), or to create a new method (polluting the namespace). Either way, make a way to remove all edges between two nodes.
  3. Implement this in NetworkX. To change the current function's behavior, we would need a special case to handle removing edges from ebunch that are 2-tuples. In that case call super's remove_edge method where the super is sufficiently far up that it is not MultiGraph anymore. I always have to think hard about super, and I haven't, but I think this should work:
super(MultiGraph, self).remove_edge(u, v)

So, we need a PR for 1) and a separate PR for 3). Maybe we can decide 2) in the discussion here.

@rossbar rossbar added this to the networkx-2.7 milestone Feb 3, 2022
@rossbar
Copy link
Contributor

rossbar commented Feb 3, 2022

I've added the milestone for the documentation component (i.e. point 1) above) which is clearly incorrect and should be fixed before the next release. The other points may require separate discussion --- in fact, it may make sense to break out @dschult 's comment into a separate issue or discussion topic, but we can cross that bridge when we come to it!

@dschult
Copy link
Member

dschult commented Feb 4, 2022

To address 2) above:
I think I favor the "same namespace" approach hinted at in 3) above:
The way to remove all the edges between u and v in a multi(di)graph is: G.remove_edges_from([(u, v)])

The current behavior for this idiom is to remove a single arbitrary edge between u and v. This makes remove_edges_from behave as the doc_string says it should. So, this behavior is backward incompatible. So we'd need to make a Big Fuss in the release notes. But it is unlikely to affect many users or we would have heard about the documentation error at some point in the last 12 years :} If you want to remove an arbitrary multiedge between u and v, you can use G.remove_edge(u, v). So the old behavior is not hard to obtain.

@rossbar
Copy link
Contributor

rossbar commented Jun 27, 2022

The documentation update was closed by #5699. I've moved the remaining points for discussion to #5825 so we can continue the discussion there.

@rossbar rossbar closed this as completed Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants