Add a topologicalSort function #66
Conversation
| List<T> topologicalSort<T>(Iterable<T> nodes, Iterable<T> Function(T) edges, | ||
| {bool Function(T, T)? equals, int Function(T)? hashCode}) { | ||
| // https://en.wikipedia.org/wiki/Topological_sorting#Depth-first_search | ||
| var result = QueueList<T>(); |
There was a problem hiding this comment.
Not sure its worth the dependency just to be able to return something that implements List here? I will defer to @natebosch though. It is likely that people will end up calling toList on this if we give them an Iterable, so this is a bit more efficient in that case.
There was a problem hiding this comment.
It might also be nice to have an optional length parameter so we can set the initial capacity when it is known by the caller.
There was a problem hiding this comment.
Not sure its worth the dependency just to be able to return something that implements
Listhere? I will defer to @natebosch though. It is likely that people will end up callingtoListon this if we give them anIterable, so this is a bit more efficient in that case.
It's just from the collection package, which is already going to be included in essentially every Dart project and effectively never has breaking changes, so what's the cost?
It might also be nice to have an optional
lengthparameter so we can set the initial capacity when it is known by the caller.
The length is always going to be the same as nodes.length, so it seems kind of silly to pass it in separately. I could just call nodes.length, but that seems potentially awkward in the case where nodes isn't an EfficientLengthIterable. We could also call nodes.toList() before accessing, but at that point we're double-allocating anyway to the benefit of knowing the length ahead of time is minimal. If we had dart-lang/sdk#29862 that would solve the issue, but in the absence of that maybe I should just do:
| var result = QueueList<T>(); | |
| var result = QueueList<T>(length: | |
| nodes is List<T> || nodes is Set<T> ? nodes.length : null); |
WDYT?
There was a problem hiding this comment.
I think it's fine to have the dependency on collection. I wouldn't want to expose that through the return type, but as is I don't think it's a probably to use it.
I'm trying to think through whether we should keep the return type Iterable anyway. @nex3 - is there a particular reason you'd need a List or would Iterable return satisfy your use case?
The length is always going to be the same as
nodes.length,
Are all nodes required to be passed in through nodes? It looks like it might work to pass in the roots of the graph.
There was a problem hiding this comment.
The length is always going to be the same as
nodes.length, so it seems kind of silly to pass it in separately. I could just callnodes.length, but that seems potentially awkward in the case wherenodesisn't anEfficientLengthIterable.
Ya I suggested another argument just because nodes isn't necessarily an EfficientLengthIterable, but I agree it feels a bit awkward to pass in the length as an optional thing.
Are all nodes required to be passed in through
nodes? It looks like it might work to pass in the roots of the graph.
Ya this is a bit interesting in general... I wonder if we should be doing some sort of check. The current behavior I think would return you a list with all the reachable nodes from nodes, topologically sorted, which probably isn't the desired behavior. You would probably want it to return a list of only the nodes it was given.
There was a problem hiding this comment.
You would probably want it to return a list of only the nodes it was given.
None of the other methods require passing in a complete list of nodes, they all allow passing roots and discovering remaining nodes through the edges.
There was a problem hiding this comment.
I'm trying to think through whether we should keep the return type
Iterableanyway. @nex3 - is there a particular reason you'd need aListor wouldIterablereturn satisfy your use case?
My use-case would prefer a list, and I also think in general it's better to accept iterables and return lists as a rule of thumb since lists are returned iterables often require copying to use in many circumstances.
Are all nodes required to be passed in through
nodes? It looks like it might work to pass in the roots of the graph.
That's a good point, I've updated the documentation to accommodate. Given that I guess it makes a bit more sense to add a length parameter, but I feel like it's unlikely to be all that useful practice. If it were me, I'd wait until someone actually requested it.
There was a problem hiding this comment.
Ya I don't have a strong opinion here, it probably doesn't really matter enough to add.
There was a problem hiding this comment.
If it were me, I'd wait until someone actually requested it.
+1
I've updated the documentation to accommodate.
LGTM once the docs are updated.
lib/graphs.dart
Outdated
| @@ -3,6 +3,8 @@ | |||
| // BSD-style license that can be found in the LICENSE file. | |||
|
|
|||
| export 'src/crawl_async.dart' show crawlAsync; | |||
| export 'src/cycle_exception.dart'; | |||
There was a problem hiding this comment.
[nit] Add explicit show to these exports.
| List<T> topologicalSort<T>(Iterable<T> nodes, Iterable<T> Function(T) edges, | ||
| {bool Function(T, T)? equals, int Function(T)? hashCode}) { | ||
| // https://en.wikipedia.org/wiki/Topological_sorting#Depth-first_search | ||
| var result = QueueList<T>(); |
There was a problem hiding this comment.
If it were me, I'd wait until someone actually requested it.
+1
I've updated the documentation to accommodate.
LGTM once the docs are updated.
|
This is now published. https://pub.dev/packages/graphs/versions/2.1.0 |
No description provided.