Conversation
CodSpeed Performance ReportMerging #190 will not alter performanceComparing Summary
|
| /* | ||
| TODO K4liber: description | ||
|
|
||
| Finds all cycles using Johnson's algorithm. |
There was a problem hiding this comment.
🤔 What's the advantage here of Johnson's algorithm over just using the existing pathfinding function with from_modules set equal to to_modules? E.g. why not find_shortest_path_bidirectional(&graph, &start_modules, &start_modules, ...) - I think that should work? Adding a new algorithm like this adds quite some complexity that needs to be maintained - especially since your algorithm basically transforms the graph to build an entirely new data structure (_DirectedGraph).
There was a problem hiding this comment.
E.g. why not find_shortest_path_bidirectional(&graph, &start_modules, &start_modules, ...) - I think that should work?
I tried this out, seems to work -> #194
| impl Graph { | ||
| pub fn find_cycles( | ||
| &self | ||
| ) -> Vec<Vec<String>> { |
There was a problem hiding this comment.
🤔 I'm not sure we want to work with String here . Inside Graph I've designed the code to work in terms of ModuleTokens. We generally translate back to strings just for the python layer (i.e. in GraphWrapper in lib.rs).
| pub fn find_cycles( | ||
| &self | ||
| ) -> PyResult<Vec<Vec<String>>> { |
There was a problem hiding this comment.
🤔 To find all cycles in the entire graph like this sounds like it could be very computationally expensive for large graphs. Should we instead just give one start module/package to work from? e.g. find_shortest_cycle(&self, module: &str, as_package: bool) -> PyResult <Option<Vec<String>>> might be a more reasonable API.
There was a problem hiding this comment.
I left a couple of comments, since this looks interesting. (Who am I and why am I commenting? 😁 - I’m the one who wrote most of this new rust graph implementation).
I’m a bit unsure if we actually want this in grimp and on the exact public API we’d want - I’d leave that part for @seddonym to consider.
(I see this is still a draft, but I thought some early feedback can still be useful - in particular it seems like we should align a bit on whether we want this and the public API, to avoid wasting your time)
|
Thanks @Peter554 for the comments. As you suggest, let's discuss the justification and a signature of the method before we go into more details. I started the conversation here: #189 (comment) |
2 participants
Solving #189