-
Notifications
You must be signed in to change notification settings - Fork 96
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
(Issue #573) Update Random Number Generation to Use NumPy Random Generators #585
(Issue #573) Update Random Number Generation to Use NumPy Random Generators #585
Conversation
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.
Hello @samadpls 👋 thank you for this PR.
By searching for np.random in the GitHub repository I found at least one other instance where deprecated random number generation is used: data/dataset/sqlite/sqlite_dataset_perturbed.py
. Line 130
Since this PR aims to remove the legacy random number generation from numpy to the new supported version I think we should also update how it's done in data/dataset/sqlite/sqlite_dataset_perturbed.py
, possibly by adding a new optional seed or a np.Generator
argument in the constructor. 😄
Let me know what you think
Hi @AMHermansen 👋, thank you for the comment, I've added an optional seed argument in the constructor. Let me know if you have any further suggestions |
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.
Looks good 😃
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.
@samadpls I stand corrected by the unittests / code styling.
I've left some comments which should hopefully make the tests pass, you could try and install pre-commit hooks as well to make sure the code quality passes 😃
@RasmusOrsoe Codeclimate is currently failing for this PR due to too high "Cognitive Complexity" should we ignore it here, or does it need refactoring? |
Hello @samadpls - thank you for this contribution! Automated checks are running; let's see what they say. The codeclimate issue is not a problem @AMHermansen. We can merge if the rest is OK. |
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.
Looks good
…rators (Issue graphnet-team#573) Update Random Number Generation to Use NumPy Random Generators
This pull request addresses issue #573 by updating the random number generation in the codebase. It replaces the deprecated use of
numpy.random.RandomState
with the recommended approach of using NumPy random generators (numpy.random.Generator
).