This repository was archived by the owner on May 24, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 15
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
/// An exception indicating that a cycle was detected in a graph that was | ||
/// expected to be acyclic. | ||
class CycleException<T> implements Exception { | ||
/// The list of nodes comprising the cycle. | ||
/// | ||
/// Each node in this list has an edge to the next node. The final node has an | ||
/// edge to the first node. | ||
final List<T> cycle; | ||
|
||
CycleException(Iterable<T> cycle) : cycle = List.unmodifiable(cycle); | ||
|
||
@override | ||
String toString() => 'A cycle was detected in a graph that must be acyclic:\n' | ||
'${cycle.map((node) => '* $node').join('\n')}'; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:collection'; | ||
|
||
import 'package:collection/collection.dart'; | ||
|
||
import 'cycle_exception.dart'; | ||
|
||
/// Returns a topological sort of the nodes of the directed edges of a graph | ||
/// provided by [nodes] and [edges]. | ||
/// | ||
/// Each element of the returned iterable is guaranteed to appear after all | ||
/// nodes that have edges leading to that node. The result is not guaranteed to | ||
/// be unique, nor is it guaranteed to be stable across releases of this | ||
/// package; however, it will be stable for a given input within a given package | ||
/// version. | ||
/// | ||
/// If [equals] is provided, it is used to compare nodes in the graph. If | ||
/// [equals] is omitted, the node's own [Object.==] is used instead. | ||
/// | ||
/// Similarly, if [hashCode] is provided, it is used to produce a hash value | ||
/// for nodes to efficiently calculate the return value. If it is omitted, the | ||
/// key's own [Object.hashCode] is used. | ||
/// | ||
/// If you supply one of [equals] or [hashCode], you should generally also to | ||
/// supply the other. | ||
/// | ||
/// Throws a [CycleException<T>] if the graph is cyclical. | ||
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>(); | ||
var permanentMark = HashSet<T>(equals: equals, hashCode: hashCode); | ||
var temporaryMark = LinkedHashSet<T>(equals: equals, hashCode: hashCode); | ||
void visit(T node) { | ||
if (permanentMark.contains(node)) return; | ||
if (temporaryMark.contains(node)) { | ||
throw CycleException(temporaryMark); | ||
} | ||
|
||
temporaryMark.add(node); | ||
for (var child in edges(node)) { | ||
visit(child); | ||
} | ||
temporaryMark.remove(node); | ||
permanentMark.add(node); | ||
result.addFirst(node); | ||
} | ||
|
||
for (var node in nodes) { | ||
visit(node); | ||
} | ||
return result; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:collection'; | ||
|
||
import 'package:graphs/graphs.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
import 'utils/utils.dart'; | ||
|
||
void main() { | ||
group('sorts a graph', () { | ||
test('with no nodes', () { | ||
expect(_topologicalSort({}), isEmpty); | ||
}); | ||
|
||
test('with only one node', () { | ||
expect(_topologicalSort({1: []}), equals([1])); | ||
}); | ||
|
||
test('with no edges', () { | ||
expect(_topologicalSort({1: [], 2: [], 3: [], 4: []}), | ||
unorderedEquals([1, 2, 3, 4])); | ||
}); | ||
|
||
test('with single edges', () { | ||
expect( | ||
_topologicalSort({ | ||
1: [2], | ||
2: [3], | ||
3: [4], | ||
4: [] | ||
}), | ||
equals([1, 2, 3, 4])); | ||
}); | ||
|
||
test('with many edges from one node', () { | ||
var result = _topologicalSort({ | ||
1: [2, 3, 4], | ||
2: [], | ||
3: [], | ||
4: [] | ||
}); | ||
expect(result.indexOf(1), lessThan(result.indexOf(2))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(3))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(4))); | ||
}); | ||
|
||
test('with transitive edges', () { | ||
var result = _topologicalSort({ | ||
1: [2, 4], | ||
2: [], | ||
3: [], | ||
4: [3] | ||
}); | ||
expect(result.indexOf(1), lessThan(result.indexOf(2))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(3))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(4))); | ||
expect(result.indexOf(4), lessThan(result.indexOf(3))); | ||
}); | ||
|
||
test('with diamond edges', () { | ||
var result = _topologicalSort({ | ||
1: [2, 3], | ||
2: [4], | ||
3: [4], | ||
4: [] | ||
}); | ||
expect(result.indexOf(1), lessThan(result.indexOf(2))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(3))); | ||
expect(result.indexOf(1), lessThan(result.indexOf(4))); | ||
expect(result.indexOf(2), lessThan(result.indexOf(4))); | ||
expect(result.indexOf(3), lessThan(result.indexOf(4))); | ||
}); | ||
}); | ||
|
||
test('respects custom equality and hash functions', () { | ||
expect( | ||
_topologicalSort<int>({ | ||
0: [2], | ||
3: [4], | ||
5: [6], | ||
7: [] | ||
}, | ||
equals: (i, j) => (i ~/ 2) == (j ~/ 2), | ||
hashCode: (i) => (i ~/ 2).hashCode), | ||
equals([ | ||
0, | ||
anyOf([2, 3]), | ||
anyOf([4, 5]), | ||
anyOf([6, 7]) | ||
])); | ||
}); | ||
|
||
group('throws a CycleException for a graph with', () { | ||
test('a one-node cycle', () { | ||
expect( | ||
() => _topologicalSort({ | ||
1: [1] | ||
}), | ||
throwsCycleException([1])); | ||
}); | ||
|
||
test('a multi-node cycle', () { | ||
expect( | ||
() => _topologicalSort({ | ||
1: [2], | ||
2: [3], | ||
3: [4], | ||
4: [1] | ||
}), | ||
throwsCycleException([1, 2, 3, 4])); | ||
}); | ||
}); | ||
} | ||
|
||
/// Runs a topological sort on a graph represented a map from keys to edges. | ||
List<T> _topologicalSort<T>(Map<T, List<T>> graph, | ||
{bool Function(T, T)? equals, int Function(T)? hashCode}) { | ||
if (equals != null) { | ||
graph = LinkedHashMap(equals: equals, hashCode: hashCode)..addAll(graph); | ||
} | ||
return topologicalSort(graph.keys, (node) { | ||
expect(graph, contains(node)); | ||
return graph[node]!; | ||
}, equals: equals, hashCode: hashCode); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callingtoList
on this if we give them anIterable
, so this is a bit more efficient in that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?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 wherenodes
isn't anEfficientLengthIterable
. We could also callnodes.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:WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aList
or wouldIterable
return satisfy your use case?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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I suggested another argument just because
nodes
isn't necessarily anEfficientLengthIterable
, but I agree it feels a bit awkward to pass in the length as an optional thing.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
LGTM once the docs are updated.