Conversation
There was a problem hiding this comment.
Why did you delete the README?
| std::unordered_map<Node, std::unordered_set<Node>> | ||
| get_predecessors(DiGraphView const &, std::unordered_set<Node> const &); | ||
|
|
||
| // return the set of nodes without incoming edges |
There was a problem hiding this comment.
Fixed in a separate commit -- pull in changes from the commit linked in #758
| DiGraphView() = delete; | ||
|
|
||
| operator GraphView() const; | ||
| operator GraphView() const { |
There was a problem hiding this comment.
Move implementations to .cc files to avoid unnecessary compilations
|
|
||
| namespace FlexFlow { | ||
|
|
||
| /** |
| GraphView(std::shared_ptr<IGraphView const>); | ||
| GraphView(std::shared_ptr<IGraphView const> ptr) : ptr(ptr) {} | ||
|
|
||
| private: |
There was a problem hiding this comment.
| private: |
There was a problem hiding this comment.
It would be great if you could review your PRs before requesting reviews (such as with git add -p) just to avoid small fixes like this making it in to the review (if you're already doing this, no problem--we all miss things sometimes 🙂)
There was a problem hiding this comment.
Also why are we making this public? I don't see a reason why this shouldn't be private.
|
|
||
| struct IGraph : IGraphView { | ||
| IGraph(IGraph const &) = delete; | ||
| IGraph() = default; |
| InputMultiDiEdge(std::pair<std::size_t, std::size_t> const &, | ||
| Node const &, | ||
| std::size_t const &); | ||
| InputMultiDiEdge(std::pair<std::size_t, std::size_t> const &uid, |
There was a problem hiding this comment.
Implementations in .cc files
|
|
||
| namespace FlexFlow { | ||
|
|
||
| struct UndirectedEdge : public use_visitable_cmp<UndirectedEdge> { |
There was a problem hiding this comment.
I generally prefer the explicit public--if you have an reason to prefer the other though I'd be happy to be convinced the other way
|
|
||
| private: | ||
| UndirectedGraphView(std::shared_ptr<IUndirectedGraphView const>); | ||
| UndirectedGraphView(std::shared_ptr<IUndirectedGraphView const> ptr) |
There was a problem hiding this comment.
Why is this being made public?
| UndirectedGraph &operator=(UndirectedGraph); | ||
|
|
||
| operator UndirectedGraphView() const; | ||
| operator UndirectedGraphView() const { |
There was a problem hiding this comment.
| template <> | ||
| struct hash<::FlexFlow::JoinNodeKey> { | ||
| std::size_t operator()(::FlexFlow::JoinNodeKey const &) const; | ||
| std::size_t operator()(::FlexFlow::JoinNodeKey const &key) const { |
There was a problem hiding this comment.
Instead convert JoinNodeKey to being visitable and use MAKE_VISIT_HASHABLE
| maybe_owned_ref() = delete; | ||
| maybe_owned_ref(T *); | ||
| maybe_owned_ref(std::shared_ptr<T>); | ||
| maybe_owned_ref(T *ptr) : _ptr(std::shared_ptr<T>(ptr)){}; |
There was a problem hiding this comment.
In this case the pointer should not be owned: change to _ptr(ptr)
| } | ||
|
|
||
| std::unordered_set<Node> get_nodes(GraphView const &g) { | ||
| return g.unsafe()->query_nodes({}); |
There was a problem hiding this comment.
| return g.unsafe()->query_nodes({}); | |
| return g.query_nodes({}); |
Also, get_nodes(IGraphView const &) should be removed--algorithms should now function on Graphs, not IGraphs
| } | ||
|
|
||
| std::unordered_set<Node> get_sinks(MultiDiGraphView const &g) { | ||
| std::unordered_set<Node> dsts; |
There was a problem hiding this comment.
Maybe change to use filter from containers.h? Would shorten this quite a bit I think
| return std::unique_ptr<IDiGraphView>(new ViewMultiDiGraphAsDiGraph{multidi}); | ||
| } | ||
|
|
||
| // DiGraphView unsafe_view_as_digraph(MultiDiGraphView const &) { |
| assert(lhs.srcs.has_value() && lhs.dsts.has_value() && rhs.srcs.has_value() && | ||
| rhs.dsts.has_value()); | ||
|
|
||
| tl::optional<std::unordered_set<Node>> srcs_t1 = |
There was a problem hiding this comment.
| tl::optional<std::unordered_set<Node>> srcs_t1 = | |
| optional<std::unordered_set<Node>> srcs_t1 = |
|
|
||
| DirectedEdgeQuery query_intersection(DirectedEdgeQuery const &lhs, | ||
| DirectedEdgeQuery const &rhs) { | ||
| assert(lhs.srcs.has_value() && lhs.dsts.has_value() && rhs.srcs.has_value() && |
There was a problem hiding this comment.
This assertion should be removed: see https://github.com/flexflow/FlexFlow/blob/6ef694251aea7b4c655f95cdbd4fb1e39e4bbcc5/lib/utils/src/graph/undirected.cc#L14-L24 or the README
| tl::optional<std::unordered_set<Node>> dsts = | ||
| intersection(*lhs.dsts, *rhs.dsts); | ||
|
|
||
| // TODO, how to set srcIdxs, dstIdxs |
There was a problem hiding this comment.
Let's not merge this change until this TODO has been resolved
|
|
||
| MultiDiGraph::operator MultiDiGraphView() const { | ||
| std::shared_ptr<IMultiDiGraph const> sharedPtr = ptr.get_shared_ptr(); | ||
| return MultiDiGraphView(sharedPtr); |
There was a problem hiding this comment.
| return MultiDiGraphView(sharedPtr); | |
| return MultiDiGraphView(this->ptr.get_shared_ptr()); |
| @@ -18,9 +18,16 @@ struct SplitASTNode; | |||
| using SplitAST = mpark::variant<SplitASTNode, Node>; | |||
|
|
|||
| struct SplitASTNode { | |||
There was a problem hiding this comment.
Move implementation to .cc file (also make SplitASTNode visitable)
| JoinNodeKey::JoinNodeKey(Node const &node, LRDirection direction) | ||
| : node(node), direction(direction) {} | ||
|
|
||
| bool JoinNodeKey::operator==(JoinNodeKey const &jnk) const { |
There was a problem hiding this comment.
Make JoinNodeKey visitable and use the automatic == generation
| CHECK_BEFORE(3, 4); | ||
| CHECK_BEFORE(4, 5); | ||
| } | ||
| // #include "doctest.h" |
There was a problem hiding this comment.
Why comment out all of the tests?
lockshaw
left a comment
lockshaw
left a comment
There was a problem hiding this comment.
See individual comments
2 participants
add some method for utils