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]>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ graph-tool mailing list [email protected] http://lists.skewed.de/mailman/listinfo/graph-tool
