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

Refactor edmonds algorithm #6743

Merged
merged 22 commits into from Jul 3, 2023
Merged

Conversation

mjschwenne
Copy link
Contributor

@mjschwenne mjschwenne commented Jun 13, 2023

Hello!

I've finished my refactor of Edmond's optimal branching algorithm so it is not dependent on the Edmonds class or the MultiDiGraph_EdgeKey class. The algorithm itself now lives under maximum_branching and the other branching and arborescence functions transform the edge weights and use the maximum_branching function.

Things left to do

There are two things that still need to happen here.

  1. I would like to deprecate the Edmonds class as well as MultiDiGraph_EdgeKey and the get_path function in branching.py. These classes (and function) are no longer required and always struck me as odd since the rest of the nx codebase doesn't use objects to run algorithms like this. However, I don't know how to deprecate anything in networkx and would appropriate some help getting that setup.
  2. As mentioned in algorithms.tree.branchings breaks for all minimum_branching() calls #4836, the minimum_branching function will return an empty branching for graphs with only positive edge weights since it can choose to not add an edge which will move it further away from the minimum or maximum value. This isn't helpful, isn't documented and should be corrected. I can think of two ways to do this, either
    • Create a new function called minimal_branching which uses the same weight transformation as minimum_spanning_arborescence but doesn't throw an error when the result isn't an arborescence. This will let us return a branching which will be like an arborescence but doesn't have to span the whole graph and would be useful when the graph has multiple components.
    • Change the behavior of minimum_branching since it really isn't useful right now, although it would require some form of warning to the user that the behavior has changed. I'm not really sure if that's truly a deprecation or something less.

@dschult
Copy link
Member

dschult commented Jun 23, 2023

As for "how to do a deprecation", that sounds like something that would be a good section in the contributors guide. I'll try to put down what I understand to be the process here and use this PR as a guinea pig if you are willing.

Deprecating code:

  • running the code should raise a DeprecationWarning with a message that says how to update the code to avoid the deprecation (if such a method exists) and which release the deprecated code will be removed. This process typically involves:
    • import warnings somewhere appropriate (at the top of the module or in the function if only one function
    • warnings.warn(msg, warnings.DeprecationWarning) but I think raise warnings.DeprecationWarning(msg) also works and I'm not sure which is preferred.
    • add a test that the warning occurs when calling that feature.
    • add the warnings filter to conftest.py in the function set_warnings (other examples are in that code).
    • add a couple lines to the deprecations section of doc/release/release_dev.py to show which PR, which function(s) and enough info to help a user looking for a solution. Look at previous release notes, e.g. release_2.8.rst for examples.
    • add a line to doc/developer/deprecations.rst to remind us to remove the code when the time comes to do that. The guidelines for picking a release to remove the code are in this file at the top.

Ask is anything isn't clear or doesn't work. :}

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 good! :)
Thanks Matt!

@dschult
Copy link
Member

dschult commented Jun 29, 2023

Also, since this is a new function, can you please make the keyword arguments "keyword only" by adding a *, between the positional arguments and the keyword arguments?

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.

A couple minor suggestions but otherwise LGTM! I'm not too familiar with the Edmond's algorithm so (as always) I rely on the expertise of others :)

networkx/algorithms/tree/branchings.py Outdated Show resolved Hide resolved
networkx/algorithms/tree/branchings.py Show resolved Hide resolved
networkx/algorithms/tree/branchings.py Outdated Show resolved Hide resolved
networkx/algorithms/tree/tests/test_branchings.py Outdated Show resolved Hide resolved
networkx/algorithms/tree/branchings.py Outdated Show resolved Hide resolved
@mjschwenne mjschwenne linked an issue Jun 30, 2023 that may be closed by this pull request
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 @mjschwenne ! Just one last note about the summary in the new docstring, but other than that this is good to go.

networkx/algorithms/tree/branchings.py Outdated Show resolved Hide resolved
@dschult dschult merged commit ab85bb9 into networkx:main Jul 3, 2023
37 checks passed
@mjschwenne mjschwenne deleted the refactor-edmonds branch July 3, 2023 17:28
@jarrodmillman jarrodmillman added this to the 3.2 milestone Jul 20, 2023
Alex-Markham pushed a commit to Alex-Markham/networkx that referenced this pull request Oct 13, 2023
* prep for edmonds refactor

* rewrite function initalization

* style update

* complete algorithm through step I2

* finished rewrite of step I3

* passing max branching tests

* attempting to track bugs with arborescence iterators

* fixed arborescence iterator bug stemming from weight transformation

* Added minimal_branching function

* update doc and keyword only args

* Update networkx/algorithms/tree/tests/test_branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* move string in original edmonds implementation

* fix tests fails caused by keyword only args

* try to fix documentation

* Minor text/format nits.

* tweak minimal_branching documentation

---------

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
dschult pushed a commit to BrunoBaldissera/networkx that referenced this pull request Oct 23, 2023
* prep for edmonds refactor

* rewrite function initalization

* style update

* complete algorithm through step I2

* finished rewrite of step I3

* passing max branching tests

* attempting to track bugs with arborescence iterators

* fixed arborescence iterator bug stemming from weight transformation

* Added minimal_branching function

* update doc and keyword only args

* Update networkx/algorithms/tree/tests/test_branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* move string in original edmonds implementation

* fix tests fails caused by keyword only args

* try to fix documentation

* Minor text/format nits.

* tweak minimal_branching documentation

---------

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* prep for edmonds refactor

* rewrite function initalization

* style update

* complete algorithm through step I2

* finished rewrite of step I3

* passing max branching tests

* attempting to track bugs with arborescence iterators

* fixed arborescence iterator bug stemming from weight transformation

* Added minimal_branching function

* update doc and keyword only args

* Update networkx/algorithms/tree/tests/test_branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* Update networkx/algorithms/tree/branchings.py

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>

* move string in original edmonds implementation

* fix tests fails caused by keyword only args

* try to fix documentation

* Minor text/format nits.

* tweak minimal_branching documentation

---------

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

algorithms.tree.branchings breaks for all minimum_branching() calls
4 participants