I'll do these modifications and submit a PR. Best, François. 2015-05-04 12:36 GMT+02:00 Tiago de Paula Peixoto <tiago@skewed.de>:
On 03.05.2015 13:18, François wrote:
Hello,
The shortest_path and shortest_distance functions are fixed to be able to handle more than one target. In order to be be more explicit, I propose for those two functions to return a dictionary when there is more than one target. This dictionary would be keyed by target vertex. What do you think about that ?
I would actually prefer this to be a numpy array, with the same ordering as the list of targets... It seems more coherent with the input, and the dict seems wasteful.
I did a minimal working example which is there: https://gist.github.com/Fkawala/afd0a666619c1d2716a5
I guess that next steps are (1) code review, and (2) performance and/or unit tests? Is that correct ?
Yes. For unit tests is suffices if you add an example with multiple targets in the docstring (which will be run via sphinx).
Just some quick comments (not a full review):
1. You changed the name of the parameter from "target" to "targets". Since this breaks backwards compatibility with a very commonly used function, that is overwhelmingly called with only one target, I think it makes more sense to keep the parameter as "target", and mention in the docstring that it accepts *both* a singe vertex as well as an iterable.
2. In the code comments you write the case with one target as being "backwards compatibility". As per the comment above, it should be considered as a standard behavior, not just backwards compatibility.
3. You use targets=[] as a default parameter. It is dangerous to use an empty list as default parameter, since it is mutable, and hence can change in the course of the program! You should replace this with "target = None".
4. The indentation in C++ is not coherent with the rest of the program (e.g. line 114 in graph_distance.cc). You should use four spaces at each indentation level. The placement of braces is also sometimes wrong (e.g. line 219).
Also, before submitting a merge request, please merge all "bugfix" and such trivial commits into self-contained commits with a clear descriptions.
Best, Tiago
-- Tiago de Paula Peixoto <tiago@skewed.de>
_______________________________________________ graph-tool mailing list graph-tool@skewed.de http://lists.skewed.de/mailman/listinfo/graph-tool
-- François Kawala