-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add use_memoise helper to memoise chull fct on the fly #80
Changes from all commits
ca52076
b6f87e4
a31c964
e47fc0c
ec60d07
2b7bc37
eb169c1
d8bc50f
a82b8ca
3e8219b
5d902a9
2535ec2
d56a970
1475a3e
21a1f39
33509b8
61c4576
15ab2bc
899ab3e
7c9ea13
4b89657
e1976ee
6f4160d
2e4a8a1
82b297a
b42284e
114e95e
246c196
fffbaab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# Workflow derived from https://github.com/r-lib/actions/tree/v2/examples | ||
# Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help | ||
on: | ||
push: | ||
paths: | ||
- README.Rmd | ||
|
||
name: Render README | ||
|
||
jobs: | ||
render-rmarkdown: | ||
runs-on: ubuntu-latest | ||
env: | ||
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} | ||
steps: | ||
- name: Checkout repo | ||
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 0 | ||
|
||
- uses: r-lib/actions/setup-pandoc@v2 | ||
|
||
- uses: r-lib/actions/setup-r@v2 | ||
|
||
- uses: r-lib/actions/setup-renv@v2 | ||
|
||
- name: Render Rmarkdown files and Commit Results | ||
run: | | ||
RMD_PATH=($(git diff --name-only ${{ github.event.before }} ${{ github.sha }} | grep '[.]Rmd$')) | ||
Rscript -e 'for (f in commandArgs(TRUE)) if (file.exists(f)) rmarkdown::render(f)' ${RMD_PATH[*]} | ||
git config --local user.name "$GITHUB_ACTOR" | ||
git config --local user.email "[email protected]" | ||
git commit ${RMD_PATH[*]/.Rmd/.md} -m 'Re-build Rmarkdown files' || echo "No changes to commit" | ||
git push origin || echo "No changes to commit" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
#' Options for \pkg{fundiversity} | ||
#' | ||
#' The memoisation is the convex hull computation in \pkg{fundiversity} is | ||
#' controlled via the `fundiversity.memoise` option: | ||
#' - if unset, the default is to use memoisation if \pkg{memoise} was installed | ||
#' when \pkg{fundiversity} was loaded, and not to use memoisation otherwise. | ||
#' - if `options(fundiversity.memoise = TRUE)`, memoisation is used and an error | ||
#' is thrown if \pkg{memoise} is not installed. | ||
#' - if `options(fundiversity.memoise = FALSE)`, memoisation is not used. | ||
#' | ||
#' @name fundiversity-options | ||
NULL | ||
|
||
#' @keywords internal | ||
use_memoise <- function() { | ||
|
||
# Cannot use memoise in parallel settings | ||
if (!inherits(future::plan(), "sequential")) { | ||
return(FALSE) | ||
} | ||
|
||
# explicitly set to TRUE by user | ||
if (isTRUE(getOption("fundiversity.memoise"))) { | ||
if (exists("fd_chull_memoised")) { | ||
return(TRUE) | ||
} | ||
stop( | ||
"memoise is not installed ", | ||
"or was installed after fundiversity was loaded. ", | ||
"Please install memoise and restart R.", | ||
call. = FALSE | ||
) | ||
} | ||
# explicitly set to FALSE by user | ||
if (isFALSE(getOption("fundiversity.memoise"))) { | ||
return(FALSE) | ||
} | ||
# unspecified / default | ||
# TRUE or FALSE depending on whether memoise was installed when fundiversity | ||
# was loaded | ||
return(exists("fd_chull_memoised")) | ||
} | ||
|
||
# Added this to make 'testthat::local_mocked_bindings()' work | ||
# in 'test-use_memoise.R' | ||
exists <- NULL | ||
Rekyt marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,6 @@ | ||
# Diverse utility functions | ||
|
||
# Memoized version of fd_chull loaded if package is installed | ||
.onLoad <- function(libname, pkgname) { | ||
if (requireNamespace("memoise", quietly = TRUE) && | ||
isTRUE(getOption("fundiversity.memoise", TRUE))) { | ||
fd_chull <<- memoise::memoise(fd_chull) | ||
fd_chull_intersect <<- memoise::memoise(fd_chull_intersect) | ||
if (requireNamespace("memoise", quietly = TRUE)) { | ||
fd_chull_memoised <<- memoise::memoise(fd_chull) | ||
fd_chull_intersect_memoised <<- memoise::memoise(fd_chull_intersect) | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Do we need to warn users here?
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.
Yes we should!
A warning or a message?
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.
It should be a message, but then the risk is because this is triggered each time
use_memoise()
is used. We wouldn't want for the user to be swamped by such messages when running in parallel.Do you know a way to deal with that in base R?
I don't want to add more dependencies to fundiversity...
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.
My first intuition would be to have a logical variable
warned_future_memoise
in an internal environment that starts atFALSE
and then is set toTRUE
once the message has been printed once.It seems like overengineering a little bit though and I'm not completely sure how it would work in parallel (is the package attached each time in each new worker?) 🤔
Maybe we can just document it very clearly somewhere and leave it as is. It's probably not an actionable piece of info for the users anyways.
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.
I'll document as thoroughly as possible everywhere I find it useful.