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

Set n_batches by default #327

Merged
merged 25 commits into from
Feb 1, 2023
Merged

Set n_batches by default #327

merged 25 commits into from
Feb 1, 2023

Conversation

martinju
Copy link
Member

@martinju martinju commented Jan 20, 2023

Introduces n_batches = NULL by default in explain().
The larger n_batches is, the less memory is consumed, but the computation time also increases slightly.
Setting n_batches = NULL (the default) now chooses a reasonable default with a trade-off between computation speed and memory consumption. This default behavior will be updated later when a full performance comparison is ran, also including number of cores when parallelizing.
Note that n_batches is also used for parallelization and for progressbar.

Other edits

  • Fixed a problem with the empirical approach not releasing memory after repeated calls to the same Rcpp-function. For large n_batches this had a significant negative effect on the memory consumption (larger than for n_batches=1). Not sure why this happens, but forcing a delete with rm() + call to gc() solve the issue.
  • timing of each part of the code in explain (with new argument timing (T/F) which disables this (required for reproducibility when testing))
  • Checks that n_batches and n_combinations are compatible.
  • Some updates to error messages.

TODO in this PR:

  • Add tests for the new n_batches behavior
  • Showcase progressbar in the vignette

@martinju martinju marked this pull request as draft January 20, 2023 14:52
@martinju martinju marked this pull request as ready for review February 1, 2023 12:31
@martinju martinju merged commit 5cd9f40 into devel Feb 1, 2023
@martinju martinju deleted the n_batches_defaults branch February 1, 2023 12:33
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.

1 participant