refactor: modifications and additional tests for weighted distance #5305
Conversation
|
Rather than a boolean input using a fixed attribute to hold the weights, we want this to allow input of the attribute name that should be used as a weight, and even allow a weight function. See the treatment done for weight arguments in the |
|
Gentle ping @lucasmccabe - this looks to be on the right track, the main thing that needs to be handled is updating the API to handle a user-specified weight attribute rather than baking-in the attribute name. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
This looks good! Thanks. I've got a couple of concerns on the handling of extrema_bounding (see below) and the same comments arise for eccentricity so I didn't repeat them. Basically, we don't want to worry about rounding the results. And we probably want to use the generic shortest_path_length so it can handle whether to compute with weights or not.
Removing the rounded values in the code might impact the tests, but probably not for these simple examples.
|
I resolved some conflicts with the main branch (which removed deprecated code). So you will need to pull the changes from your fork to your local machine before you edit your files more. Hope that's not trouble and if you need help sorting that out let us know. |
- remove extraneous G_succ - invoke shortest_path_length in _extrema_bounding and eccentricity - add distance_method arg to _extrema_bounding, diameter, periphery, radius, and center - weighted tests use integer weights - remove precision-handling for float weights
- revert to float-weighted tests - remove distance_method argument - docs update re: weights
|
This looks good. I checked out how the documentation would look after this PR. You can do that by clicking on "Show all checks" on the right side of the box that says "All checks have passed". Then scroll down to the 3rd to last check which is run using "CircleCI" and says "document artifact". Click on the "Details" button and you are brought to a version of the webpage for documentation as it will be after this PR. It's a little bit of clicking to find your function: Reference (at the top) then algorithms and "distance measures". And then you can look at each function and how it renders wit its doc_string. All your doc_strings look good. But I do see that "resistance_distance" has some unformatted notes at the bottom. Would you be willing to change the formatting near lines 672. After the "Overview discussion:" we need a blank line and then indent the two bullet lines below it. Similarly, after the heading "Additional Details:", only those also need a star in front to make a bullet in the webpage. If you have had enough, just say so and we'll stop here and merge this. The other change can be done in another PR. Thanks very much! Job well done! :} |
impacted: resistance_distance and __extrema_bounding
Great! Thanks for the review, @dschult.
I see what you mean. In the most recent commit, I address this and make a similar adjustment in
I'm not sure what to make of the three cancelled checks. They seem to all be for Mac OS w/ 3.8. The only differences between 072b960 and d90a41f are the docstring updates. Perhaps it has something to do with this? |
* Add test for nice exception. * Add input validation to geometric_edges. * Fix None identity checking. Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Dan Schult <dschult@colgate.edu>
Fixes the formulae used to calculate gain and removal cost in calculation of one level of the Louvain partition tree. Errors here were causing infinite loops for some cases in the past, see networkxgh-5175 and networkxgh-5704. This PR also adds test cases to ensure infinite loops are not entered for these cases. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
* Add more comprehensive tests for pydot * remove the second element from frozenset for cleaner tests Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
* make lazy_import private and remove its internal use * add back the non-lazy imports of numpy to utils.misc
…GHA (networkx#5805) Fixes installation of pygraphviz on macOS-latest in github actions
|
I think the tests just timed out on the hardware they were use. (There are time limits for some donated hardware.) I have restarted the first and it did not cancel this time. Will restart the other two and hope for the best. |
Ah, I see. Thanks for your help! |
There was a problem hiding this comment.
This is really good -- Thanks very much.
I only have one comment/question. In a few of the functions, you call with the first input value G=G instead of just G. The function signature doesn't provide a keyword argument for the first input. It's just G. It reminds me that recent versions of Python allow function arguments to be keyword-only or position-only. Is there a reason you used this syntax? I don't know the implications.
There was a problem hiding this comment.
It looks like we're on the right track!
My main concern is about the default value of the new weight kwarg - see my comment below for details.
I agree with @dschult - we should get rid of the G=G pattern in the calls.
Finally, there should be some test cases of the new weight argument - the added tests all assume the default value.
|
Ack -- looks like you merged with the v2.8 branch instead of the main branch. The changing of the default weight looks good. |
…stance-issue-5257" This reverts commit f10b12f.
Good point - I've added two more edge attributes in the tests: "cost" (which has the same value as "weight") and "high_cost" (which is 10x "cost"). I test that using weight="cost" produces the same results as weight="weight," then test that I get the appropriate results when using weight="high_cost" (i.e. distances like
My mistake. I reverted the commit that merged with v2.8 and made a separate commit merging with main. |
There was a problem hiding this comment.
This looks good to me!
Thanks @lucasmccabe for sticking with all our requests for changes.
There was a problem hiding this comment.
Thanks @lucasmccabe ! I took the liberty of making a few final cosmetic changes, but everything else LGTM
Adds the weight keyword argument to allow users to compute weighted distance metrics e.g. diameter, eccentricity, periphery, etc. The kwarg works in the same fashion as the weight param for shortest paths - i.e. if a string, look up with edge attr by key, if callable, compute the weight via the function. Default is None, meaning return unweighted result which is the current behavior. Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Adds the weight keyword argument to allow users to compute weighted distance metrics e.g. diameter, eccentricity, periphery, etc. The kwarg works in the same fashion as the weight param for shortest paths - i.e. if a string, look up with edge attr by key, if callable, compute the weight via the function. Default is None, meaning return unweighted result which is the current behavior. Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Fixes #5257
Additional tests have been added for weighted graph distance computations.