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

Update multigraph docstrings to reflect remove_edges_from behavior. #5699

Merged
merged 3 commits into from Jun 8, 2022

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Jun 7, 2022

Addresses the first bullet of issue #5158 , i.e. updating the documentation to correct the stated behavior of remove_edges_from when 2-tuples are passed. The remaining parts of the issue are larger discussions that should probably be moved to their own topic.

@jarrodmillman jarrodmillman added this to the networkx-3.0 milestone Jun 7, 2022
@dschult
Copy link
Member

dschult commented Jun 7, 2022

This has clear implications for the doc_string of remove_edge as well. This remove_edges_from doc_string states clearly that it is the most recently added multiedge that is removed. While the remove_edge doc_string says in more than one place that it is an "arbitrary" edge that is removed. I think the difference is due to dict's now being ordered. But since we're improving this doc_string is it in scope to change it for the remove_edge doc_string as well?

Also, we should check the MultiDiGraph doc_string for remove_edges_from to make sure it is updated to match this doc_string. In this case, MultiDiGraph inherits this method. So the doc_string is identical. But that class' remove_edge doc_string will need to be updated from "arbitrary" to "most recently added".

@rossbar
Copy link
Contributor Author

rossbar commented Jun 8, 2022

But since we're improving this doc_string is it in scope to change it for the remove_edge doc_string as well?

Agreed - this should be taken care of in 1efc66d and 39a93a3. As noted, the docstring for remove_edges_from is inherited, but the docstring for remove_edges is not, so I've tried to be careful and make the appropriate changes in both MultiGraph.remove_edges and MultiDiGraph.remove_edges.

Also, we should check the MultiDiGraph doc_string for remove_edges_from to make sure it is updated to match this doc_string.

As a quick sanity check, here's the html page for the inherited docstring. AFAICT this seems to be working as expected.

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.

Looks Great! Thanks for this!

@dschult dschult merged commit c8fdab5 into networkx:main Jun 8, 2022
MridulS pushed a commit to MridulS/networkx that referenced this pull request Feb 4, 2023
…networkx#5699)

* Update MG docstring to reflect rm_edges_from behavior.

Also adds example.

* Update remove_edge docstring in MG and MDG.

* Fix MDG examples.
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
…networkx#5699)

* Update MG docstring to reflect rm_edges_from behavior.

Also adds example.

* Update remove_edge docstring in MG and MDG.

* Fix MDG examples.
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