-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fix weighted MultiDiGraphs in DAG longest path algorithms + add additional tests #5988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stevenstrickler this looks really nice. Just one suggestion about beefing up the tests by adding a new unit test for non-default values of default_weight
(ha!)
Otherwise LGTM though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stevenstrickler!
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
…ional tests (networkx#5988) * Fix weighted MultiDiGraphs in dag longest path algorithms * Add tests for MultiDiGraphs in dag longest path tests * Test non default default_weight Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> * Test non default default_weight * blackify Co-authored-by: Mridul Seth <mail@mriduls.com> Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> Co-authored-by: Mridul Seth <git@mriduls.com>
…ional tests (networkx#5988) * Fix weighted MultiDiGraphs in dag longest path algorithms * Add tests for MultiDiGraphs in dag longest path tests * Test non default default_weight Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> * Test non default default_weight * blackify Co-authored-by: Mridul Seth <mail@mriduls.com> Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> Co-authored-by: Mridul Seth <git@mriduls.com>
Fixes #5987
Personally, I would find it useful to return the edge that was calculated to be the best in addition to the node to avoid having to recalculate it again whenever traversing the path, but that would break backwards compatibility with code that is using MultiDiGraphs for unweighted longest pathing by changing the return type, so I have not included it in my fix. A potential other fix would be to create separate functions for MultiDiGraphs and add a warning to dag_longest_path and dag_longest_path_length when a MultiDiGraph is supplied—preserving backwards compatibility while notifying developers about the function's inability to compute with weights.