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

Make seed independent of num.threads and add legacy option #1447

Merged
merged 23 commits into from
Sep 25, 2024

Conversation

erikcs
Copy link
Member

@erikcs erikcs commented Sep 22, 2024

This PR makes the random seed argument produce the same results irrespective of what num.threads is set to. In order to reproduce past results, a global package option options(grf.legacy.seed) is added which can be set to TRUE to revert back to the old behavior where seed and num.threads interacts (see grf_options for an example).

#1368.

@erikcs erikcs marked this pull request as ready for review September 22, 2024 07:51
@erikcs erikcs changed the title test Make seed independent of num.threads and add legacy option Sep 22, 2024
Copy link
Member

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Great we're finally fixing this. I do wonder if the grf.legacy.seed option adds more complexity than it's worth ... it's pretty easy to install old versions of R packages, and for reproducibility it's good practice to save and fix the package version. And now, we'll have to figure out when it's save to deprecate and remove the new option grf.legacy.seed !

core/src/forest/ForestTrainer.cpp Outdated Show resolved Hide resolved
@erikcs
Copy link
Member Author

erikcs commented Sep 24, 2024

Yes it adds complexity, but that's the price we have to pay in order to do this imo! Past versions of GRF are not available as CRAN binaries and have to be installed from source, which unfortunately can be challenging for many users.

(there's a bunch of unrelated commits below because I got a new arm mac that evidently produces slightly different results for characterization tests stored on the repo using x86-based systems)

@erikcs erikcs added bug breaking Signifies a breaking change. labels Sep 24, 2024
@erikcs erikcs merged commit 7bffa33 into grf-labs:master Sep 25, 2024
1 of 7 checks passed
@erikcs erikcs deleted the legacy-seed branch September 25, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Signifies a breaking change. bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants