Add lowest_common_ancestor algorithm
#35
Conversation
eriknw
added
improvement
| __all__ = ["lowest_common_ancestor"] | ||
|
|
||
|
|
||
| @not_implemented_for("undirected") |
There was a problem hiding this comment.
Does adding this make the algorithm automatically raise errors on undirected graphs?
There was a problem hiding this comment.
Yes. It's a decorator from networkx.
We place it before @networkx_algorithm, because NetworkX already checks and raises if the input graph is undirected before dispatching to backends:
https://github.com/networkx/networkx/blob/9beaf7a0b59fe21775cd93862d9c7b28152a2d8c/networkx/algorithms/lowest_common_ancestors.py#L115-L117
In other words, we use the same decorator so the algorithm behaves correctly when used directly such as nxcg.lowest_common_ancestor(G).
| """May not always raise NetworkXError for graphs that are not DAGs.""" | ||
| G = _to_directed_graph(G) | ||
|
|
||
| # if not nxcg.is_directed_acyclic_graph(G): # TODO |
There was a problem hiding this comment.
Do we need to wait on this before merging?
There was a problem hiding this comment.
It would be nice to, but I don't think it's super important in practice so I would say no. We add a note to the docstring. If/when PLC can help answer whether a graph is a DAG, then we should add nxcg.is_directed_acyclic_graph ASAP.
The difference is that if the graph is not a DAG, then we may still give an answer or we may raise as networkx does. It's common to know whether your graph is a DAG or not (often by construction).
|
I added a basic benchmark where I force the input graph to be a DAG. The two initial nodes were chosen randomly; it would be nice to make benchmarks that take nodes a bit more robust. Here are some results:
Generally, I think relative performance should be comparable to |
eriknw
changed the title
lowest_common_ancestors algorithmlowest_common_ancestor algorithm
|
Companion PR that updates docs: rapidsai/cugraph#4779 |
This is a medium-priority algorithm that I considered pairing with @nv-rliu on instead of #32, so I thought I would knock it out while it was still fresh in mind. It was pretty quick.
To reviewers: this is low priority and may slip to the next release. Please do more important things first. It is reasonable to request benchmarking, which I hope to add when I get a chance.
I created more tests in networkx to help with this PR: networkx/networkx#7726
I think the implementation is pretty clean and straightforward, so hopefully others think so too (although it probably requires understanding what this algorithm does).