-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix directed gnp_random_graph #954
Changes from 18 commits
4e869f4
9308f1a
faf391c
876f76e
dfaa1bb
50e1e68
361b128
150e3d6
2a2c131
85ac71a
2338ec9
6a00d6a
01a87d8
ac5541a
acaa7b7
12e917f
4ebc900
500a1e0
fd7544a
5d387b1
b41cf8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
fixes: | ||
- | | ||
Fixed an issue where the :func:`~rustworkx.generators.directed_gnp_random_graph` | ||
and the :func:`~rustworkx-core.generators.gnp_random_graph` for directed graphs | ||
produced a graph where lower node numbers had only a small number of edges | ||
compared to what was expected and the output was therefore not random. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,7 @@ where | |
if !(0.0..=1.0).contains(&probability) { | ||
return Err(InvalidInputError {}); | ||
} | ||
|
||
if probability > 0.0 { | ||
if (probability - 1.0).abs() < std::f64::EPSILON { | ||
for u in 0..num_nodes { | ||
|
@@ -113,32 +114,46 @@ where | |
} | ||
} | ||
} else { | ||
let mut v: isize = if directed { 0 } else { 1 }; | ||
let mut w: isize = -1; | ||
let num_nodes: isize = num_nodes as isize; | ||
let num_nodes = num_nodes as isize; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is pre-existing code but couldn't this potentially overflow since the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, should we say something in the docs about num_nodes not exceeding some value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should probably just error in the case we've exceeded. We can document it but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in fd7544a. I believe this does what you're saying. I wasn't sure how to test this, since anything I tried with very large numbers gave either a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I redid this in b41cf8a using match. I was able to successfully test locally if I changed all the |
||
let lp: f64 = (1.0 - probability).ln(); | ||
|
||
let between = Uniform::new(0.0, 1.0); | ||
while v < num_nodes { | ||
let random: f64 = between.sample(&mut rng); | ||
let lr: f64 = (1.0 - random).ln(); | ||
let ratio: isize = (lr / lp) as isize; | ||
w = w + 1 + ratio; | ||
|
||
if directed { | ||
// avoid self loops | ||
// For directed, create inward edges to a v | ||
if directed { | ||
let mut v: isize = 0; | ||
let mut w: isize = -1; | ||
while v < num_nodes { | ||
let random: f64 = between.sample(&mut rng); | ||
let lr: f64 = (1.0 - random).ln(); | ||
w = w + 1 + ((lr / lp) as isize); | ||
while w >= v && v < num_nodes { | ||
w -= v; | ||
v += 1; | ||
} | ||
// Skip self-loops | ||
if v == w { | ||
w += 1; | ||
w -= v; | ||
v += 1; | ||
} | ||
if v < num_nodes { | ||
let v_index = graph.from_index(v as usize); | ||
let w_index = graph.from_index(w as usize); | ||
graph.add_edge(w_index, v_index, default_edge_weight()); | ||
} | ||
} | ||
while v < num_nodes && ((directed && num_nodes <= w) || (!directed && v <= w)) { | ||
} | ||
|
||
// For directed and undirected, create outward edges from a v | ||
// Nodes in graph are from 0,n-1 (start with v as the second node index). | ||
let mut v: isize = 1; | ||
let mut w: isize = -1; | ||
while v < num_nodes { | ||
let random: f64 = between.sample(&mut rng); | ||
let lr: f64 = (1.0 - random).ln(); | ||
w = w + 1 + ((lr / lp) as isize); | ||
while w >= v && v < num_nodes { | ||
w -= v; | ||
v += 1; | ||
// avoid self loops | ||
if directed && v == w { | ||
w -= v; | ||
v += 1; | ||
} | ||
} | ||
if v < num_nodes { | ||
let v_index = graph.from_index(v as usize); | ||
|
@@ -408,7 +423,7 @@ mod tests { | |
let g: petgraph::graph::DiGraph<(), ()> = | ||
gnp_random_graph(20, 0.5, Some(10), || (), || ()).unwrap(); | ||
assert_eq!(g.node_count(), 20); | ||
assert_eq!(g.edge_count(), 104); | ||
assert_eq!(g.edge_count(), 189); | ||
} | ||
|
||
#[test] | ||
|
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.
Wasn't it still technically random output, just with a much lower occurrence of an edge than specified.
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.
That's fine. I'll remove the last clause.
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.
Done in fd7544a.