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

Improve test coverage for Steiner Tree & Docs #7348

Merged
merged 7 commits into from Mar 15, 2024

Conversation

vanshika230
Copy link
Contributor

I have increased test coverage for Steinertree.py according to here :-
https://app.codecov.io/gh/networkx/networkx/blob/main/networkx%2Falgorithms%2Fapproximation%2Fsteinertree.py
It is now at 100% as seen below:-
after
I have also added Raises to docstrings and changed the default from kou to mehlhorn according to recent changes.

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.

Generally LGTM, and thanks for the cleanup of things I missed in #7337 🚀

Just one note about how to structure the default method test, but other than that LGTM!

networkx/algorithms/approximation/steinertree.py Outdated Show resolved Hide resolved
networkx/algorithms/approximation/steinertree.py Outdated Show resolved Hide resolved
vanshika230 and others added 5 commits March 14, 2024 10:46
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.

This LGTM, thanks @vanshika230 !

The dispatching tests are failing but I'm not sure why... @eriknw any ideas?

@eriknw
Copy link
Contributor

eriknw commented Mar 14, 2024

The dispatching tests are failing but I'm not sure why... @eriknw any ideas?

Thanks for the ping! You can expect me to be around to help with issues like this. This is a good data point, and I suppose we should make the assertion message more informative.

EDIT: this is not the best solution. See my next reply

I debugged this locally, and the solution is to change the _dispatchable decorator of steiner_tree to:

@nx._dispatchable(edge_attrs={"weight": None}, returns_graph=True)

The issue is that "weight" edge attribute was being filled to 1 (the default value according to the docstring) in the converted backend graph, but the correct behavior is to not add edge values if they don't exist. Using edge_attrs={"weight": None} will preserve the weight edge attribute if it exists, but won't add new ones.

@eriknw
Copy link
Contributor

eriknw commented Mar 14, 2024

Correction--use this _dispatchble:

@nx._dispatchable(preserve_all_attrs=True, returns_graph=True)

The returned graph preserves edge, node, and graphs attributes from the original graph.

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.

Thanks! This is very helpful!

@dschult dschult merged commit 984f737 into networkx:main Mar 15, 2024
41 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Mar 15, 2024
@vanshika230 vanshika230 deleted the testcov_steinertree branch March 16, 2024 06:21
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Improve test coverage for Steiner Tree & Docs

* Update networkx/algorithms/approximation/steinertree.py

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

* Update networkx/algorithms/approximation/steinertree.py

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

* Update networkx/algorithms/approximation/tests/test_steinertree.py

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

* Update test_steinertree.py

* Update test_steinertree.py

* Update dispatch decorator.

---------

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
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

5 participants