Skip to content
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

Merged
merged 21 commits into from
Nov 25, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in fd7544a.

53 changes: 34 additions & 19 deletions rustworkx-core/src/generators/random_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The 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 num_nodes input is of type usize? Should we have a guard against num_nodes > isize::MAX or use try_into() here?

Copy link
Contributor Author

@enavarro51 enavarro51 Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, try_into is a good idea. If it overflows, should we set it to isize::MAX or should we error or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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 isize::MAX is 9,223,372,036,854,775,807 on 64 bit platforms and ~2 billion on 32 bit platforms, maybe just say it will return an error if the num of nodes exceeds the isize::MAX directly (for rustworkx-core). But I think the InvalidInputError error struct is probably sufficient to cover this case too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 capacity overflow or memory allocation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 isize to i8, but if I test at isize levels, I still get capacity or memory errors. So I did not add any new tests for this.

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);
Expand Down Expand Up @@ -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]
Expand Down
13 changes: 10 additions & 3 deletions tests/rustworkx_tests/digraph/test_dot.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,24 @@ def test_digraph_to_dot_to_file(self):
def test_digraph_empty_dicts(self):
graph = rustworkx.directed_gnp_random_graph(3, 0.9, seed=42)
dot_str = graph.to_dot(lambda _: {}, lambda _: {})
self.assertEqual("digraph {\n0 ;\n1 ;\n2 ;\n0 -> 1 ;\n0 -> 2 ;\n}\n", dot_str)
self.assertEqual(
"digraph {\n0 ;\n1 ;\n2 ;\n0 -> 1 ;\n0 -> 2 ;\n1 -> 2 ;\n2 -> 0 ;\n2 -> 1 ;\n}\n",
dot_str,
)

def test_digraph_graph_attrs(self):
graph = rustworkx.directed_gnp_random_graph(3, 0.9, seed=42)
dot_str = graph.to_dot(lambda _: {}, lambda _: {}, {"bgcolor": "red"})
self.assertEqual(
"digraph {\nbgcolor=red ;\n0 ;\n1 ;\n2 ;\n0 -> 1 ;\n" "0 -> 2 ;\n}\n",
"digraph {\nbgcolor=red ;\n0 ;\n1 ;\n2 ;\n0 -> 1 \
;\n0 -> 2 ;\n1 -> 2 ;\n2 -> 0 ;\n2 -> 1 ;\n}\n",
dot_str,
)

def test_digraph_no_args(self):
graph = rustworkx.directed_gnp_random_graph(3, 0.95, seed=24)
dot_str = graph.to_dot()
self.assertEqual("digraph {\n0 ;\n1 ;\n2 ;\n0 -> 1 ;\n0 -> 2 ;\n}\n", dot_str)
self.assertEqual(
"digraph {\n0 ;\n1 ;\n2 ;\n0 -> 2 ;\n1 -> 2 ;\n1 -> 0 ;\n2 -> 0 ;\n2 -> 1 ;\n}\n",
dot_str,
)
14 changes: 12 additions & 2 deletions tests/rustworkx_tests/test_random.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,20 @@


class TestGNPRandomGraph(unittest.TestCase):
def test_random_gnp_directed(self):
def test_random_gnp_directed_1(self):
graph = rustworkx.directed_gnp_random_graph(15, 0.7, seed=20)
self.assertEqual(len(graph), 15)
self.assertEqual(len(graph.edges()), 156)

def test_random_gnp_directed_2(self):
graph = rustworkx.directed_gnp_random_graph(20, 0.5, seed=10)
self.assertEqual(len(graph), 20)
self.assertEqual(len(graph.edges()), 104)
self.assertEqual(len(graph.edges()), 189)

def test_random_gnp_directed_3(self):
graph = rustworkx.directed_gnp_random_graph(22, 0.2, seed=6)
self.assertEqual(len(graph), 22)
self.assertEqual(len(graph.edges()), 91)

def test_random_gnp_directed_empty_graph(self):
graph = rustworkx.directed_gnp_random_graph(20, 0)
Expand Down