I'll do these modifications and submit a PR. Best, François.
2015-05-04 12:36 GMT+02:00 Tiago de Paula Peixoto <[email protected]>: > 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 <[email protected]> > > > _______________________________________________ > graph-tool mailing list > [email protected] > http://lists.skewed.de/mailman/listinfo/graph-tool > > -- François Kawala
_______________________________________________ graph-tool mailing list [email protected] http://lists.skewed.de/mailman/listinfo/graph-tool
