-
Notifications
You must be signed in to change notification settings - Fork 145
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
Conversation
…o gnp_single_edge
Pull Request Test Coverage Report for Build 6989824759
💛 - Coveralls |
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.
This change LGTM, just a couple of inline questions. Sorry it took so long to review this.
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.
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.
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 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?
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.
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.
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?
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.
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.
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.
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.
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.
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.
LGTM, thanks for the quick updates
This PR fixes #950. Previously the
directed_gnp_random_graph
did 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.