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