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.

Sign up for GitHub

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

Jump to bottom

gh-208: remove legacy random number generation #241

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

gh-208: remove legacy random number generation #241

wants to merge 2 commits into from

Conversation

Saransh-cpp
Copy link
Member

Gets rid of legacy RandomState and replaced it with the new random number Generator for rng calls.

Fixes: #208

@Saransh-cpp Saransh-cpp added the maintenance Maintenance: refactoring, typos, etc. label Sep 18, 2024
paddyroddy
paddyroddy requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

I think it would be good to have the random seed as a test constant or pytest.fixture

@ntessore
Copy link
Collaborator

I think it would be good to have the random seed as a test constant or pytest.fixture

Agree, I usually make rng itself the fixture, which can be helpful for testing with a mock RNG.

ntessore
ntessore requested changes Sep 19, 2024
View reviewed changes
tests/core/test_algorithm.py
Comment on lines +13 to +19
@pytest.fixture
def rng():
import numpy as np

return np.random.default_rng(seed=42)


Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this to conftest.py so it only needs to be defined once?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntessore ntessore ntessore requested changes

@paddyroddy paddyroddy Awaiting requested review from paddyroddy

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
maintenance Maintenance: refactoring, typos, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove legacy random number generation
3 participants
@Saransh-cpp @ntessore @paddyroddy