Fix directed gnp_random_graph #954
Conversation
…o gnp_single_edge
Pull Request Test Coverage Report for Build 6989824759
💛 - Coveralls |
| 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. |
There was a problem hiding this comment.
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.
That's fine. I'll remove the last clause.
There was a problem hiding this comment.
Done in fd7544a.
| 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.
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?
There was a problem hiding this comment.
Yeah, try_into is a good idea. If it overflows, should we set it to isize::MAX or should we error or both?
There was a problem hiding this comment.
Also, should we say something in the docs about num_nodes not exceeding some value?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR fixes #950. Previously the
directed_gnp_random_graphdid not properly produce in and out edges and also contained a bug that caused the process to terminate too quickly, resulting in some nodes having a very small number of edges.