clustering coefficient update #1909
Conversation
There was a problem hiding this comment.
- the path-based algorithm has room for optimisation
- we need a benchmark to decide whether it is worth keeping the set-based algorithm at all
- the filtering of nodes for the batch versions is unnecessarily inefficient (no need for creating subgraph views)
- python wrappers should raise proper errors instead of panicking
| if all_src_nodes == false { | ||
| (nodes, src_nodes) = filter_nodes(graph, &v); | ||
| g = graph.subgraph(nodes); | ||
| } else { | ||
| g = graph.subgraph(graph.nodes()); | ||
| } |
There was a problem hiding this comment.
This filter thing is rather inefficient? Most efficient is probably a Vec<bool> for the src_nodes?
| .filter_map(|nb| match g.has_edge(nb[0].id(), nb[1].id()) { | ||
| true => Some(1), | ||
| false => match g.has_edge(nb[1].id(), nb[0].id()) { | ||
| true => Some(1), | ||
| false => None, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
this should use the internal ids, not global ids (much more efficient as this version incurs unnecessary hash map lookups)
There was a problem hiding this comment.
There is also the option of considering nodes in degree order which eliminates the triple-counting of triangles and reduces the number of existence checks by quite a lot. We can then simply use atomic accumulators to keep track of the number of triangles at each node.
| if all_src_nodes == false { | ||
| (nodes, src_nodes) = filter_nodes(graph, &v); | ||
| g = graph.subgraph(nodes); | ||
| } else { | ||
| g = graph.subgraph(graph.nodes()); | ||
| } |
There was a problem hiding this comment.
same problem as the other version, filter should be more efficient
| @@ -0,0 +1,40 @@ | |||
| use crate::{core::entities::nodes::node_ref::AsNodeRef, db::api::view::*}; | |||
There was a problem hiding this comment.
I don't think the filter_nodes bit is necessary
| @@ -76,6 +77,7 @@ mod triangle_count_tests { | |||
| prelude::NO_PROPS, | |||
| test_storage, | |||
| }; | |||
| use tracing::info; | |||
| /// | ||
| /// # Returns | ||
| /// the local clustering coefficient of node v in g. | ||
| pub fn local_clustering_coefficient_batch_path<G: StaticGraphViewOps, V: AsNodeRef>( |
There was a problem hiding this comment.
We need some benchmarks, is this actually much slower than the set-based version?
| ) -> AlgorithmResult<DynamicGraph, f64, OrderedFloat<f64>> { | ||
| local_clustering_coefficient_batch_intersection_rs( | ||
| &graph.graph, | ||
| process_node_param(v).unwrap(), |
There was a problem hiding this comment.
this needs to return a PyResult!
There was a problem hiding this comment.
alternatively, we can push all of this to the pyo3 layer if we add an struct/enum which implements FromPyObject instead of the process_node_param function.
What changes were proposed in this pull request?
Refactor global and local clustering coefficient. Add two variants of batch local clustering coefficient.
Why are the changes needed?
It's currently extremely inefficient to run LCC on a group of nodes. The batch versions should do a better job of parallelizing the process and reducing overhead.
Does this PR introduce any user-facing change? If yes is this documented?
'clustering_coefficient' is renamed to 'global_clustering_coefficient'. All of the clustering coefficient variants have been moved to a submodule of 'metrics' called 'clustering_coefficient'. The new batch implementations have corresponding docstrings.
How was this patch tested?
The two methods were tested for parity against the existing implementation in Rust and Python.
Are there any further changes required?
Currently working on an approximate version that uses HyperLogLog.