implemented erdos renyl model generation #2253
Conversation
|
|
|
Apologies if I did anything wrong. This is my first PR. |
miratepuffin
left a comment
miratepuffin
left a comment
There was a problem hiding this comment.
Couple innitial thoughts
| /// let graph = Graph::new(); | ||
| /// erdos_renyi(&graph, 10, 0.2, None); | ||
| /// ``` | ||
| pub fn erdos_renyi(graph: &Graph, n_nodes: usize, p: f64, seed: Option<[u8; 32]>) { |
There was a problem hiding this comment.
This is kind of a strange seed, not possible to use just a u64?
There was a problem hiding this comment.
There was a problem hiding this comment.
yeah I'm refactoring everything atm
| .map_err(|err| error!("{:?}", err)) | ||
| .ok(); | ||
| } | ||
| let all_ids = graph.nodes().id().iter_values().collect::<Vec<_>>(); |
There was a problem hiding this comment.
You already have this above - this could be quite a big vec - would be better to reuse
There was a problem hiding this comment.
We need all the ids to be stored in a vector so the algorithm knows about all the other node ids ahead of time.
There was a problem hiding this comment.
you generate the vector twice, you already have it as ids above
| pa(&g.graph, nodes_to_add, edges_per_step, seed); | ||
| } | ||
|
|
||
| /// Generates a graph using the Erdos-Renyi random graph model. |
There was a problem hiding this comment.
Looks great! As above seed probably should be a singular number
| use crate::prelude::*; | ||
|
|
||
| #[test] | ||
| fn test_erdos_renyi_small_graph() { |
There was a problem hiding this comment.
Wanna add some tests where you add 100,000 nodes or somethign else reasonably big.
Also a good idea to look at adding some tests for graphs which already have nodes and edges present.
| .ok(); | ||
| } | ||
| let all_ids = graph.nodes().id().iter_values().collect::<Vec<_>>(); | ||
| for id in all_ids.iter() { |
There was a problem hiding this comment.
You should be able to do this in parallel over these iterators
| for other_id in all_ids.iter() { | ||
| if id != other_id && rng.gen::<f64>() < p { | ||
| latest_time += 1; | ||
| graph |
There was a problem hiding this comment.
Do we consider that an edge may already be present?
There was a problem hiding this comment.
Could I delete an edge if its present?
| for _ in 0..n_nodes { | ||
| max_id = next_id(graph, Some(max_id)); | ||
| latest_time += 1; | ||
| graph |
There was a problem hiding this comment.
ID's in raphtory can be either strings or ints - this is assuming that the graph either has no nodes or that those present are indexed by int - I think this will explode if you already have some string Ids present - should add a test for this
| #[test] | ||
| fn test_erdos_renyi_small_graph() { | ||
| let graph = Graph::new(); | ||
| let n_nodes = 5; |
There was a problem hiding this comment.
Could also add some tests to see if the graphs have expected stats e.g. density
| } | ||
| let all_ids = graph.nodes().id().iter_values().collect::<Vec<_>>(); | ||
| for id in all_ids.iter() { | ||
| for other_id in all_ids.iter() { |
There was a problem hiding this comment.
Erdos Renyi graphs are typically undirected - here you are going to loop over each edge both ways so each edge gets checked twice meaning you may get double the density you are expecting.
A better solution could be to order the id's and then only check edges for src.id < dst.id - this would also remove self loops (<= if you want to include these)
|
You might also be interested in this paper: https://kim246.wwwdns.kim.uni-konstanz.de/publications/bb-eglrn-05.pdf In section IIa they have an algorithm for generating these graphs without looping over all pairs of nodes which is going to be a lot more efficient for sparse graphs. Note that you can parallelise it by dividing the edges into chunks as the geometric distribution is memory-free. |
|
appreciate it |
4 participants
No description provided.