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

Pull Request follow-up for #30: Reset radom seed #59

Merged
merged 25 commits into from
Jul 11, 2024

Conversation

mcaceresb
Copy link
Collaborator

No description provided.

@mcaceresb
Copy link
Collaborator Author

@jonathandroth I reset the random seed here after the function's run, which was an easy mod. I can also not set the random seed internally and instead modify the code to use something like halton sequences? I wonder if CRAN will complain about a function fiddling with the global environment at all.

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 21, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth I'm not sure it's luck because the same thing happens in one function here that they also didn't flag. So I think if we put it in an argument they won't complain.

However, if the argument is not user-accessible then it has the same effect, we're just hiding it from them and going against their best practices. I guess I could make the seed user-accessible and pass it around throughout?

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 21, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth I made the seed user-accessible. I see the PR branch is passing all checks so if you're good I'll merge to main and reply in #42

@jonathandroth
Copy link
Collaborator

Sounds good to me, thanks!

@mcaceresb mcaceresb merged commit e304b8e into asheshrambachan:master Jul 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants