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

Be more specific in the help with memoization #77

Closed
Rekyt opened this issue Oct 6, 2023 · 2 comments · Fixed by #80
Closed

Be more specific in the help with memoization #77

Rekyt opened this issue Oct 6, 2023 · 2 comments · Fixed by #80
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@Rekyt
Copy link
Member

Rekyt commented Oct 6, 2023

A little connex to #71 but the current description of memoisation

fundiversity/R/fd_fric.R

Lines 34 to 41 in 2d19f5c

#' @details By default, when loading \pkg{fundiversity}, the functions to
#' compute convex hulls are
#' [memoised](https://en.wikipedia.org/wiki/Memoization) through the `memoise`
#' package if it is installed. To deactivate this behavior you can set the
#' option `fundiversity.memoise` to `FALSE` by running the following line:
#' `options(fundiversity.memoise = FALSE)`. If you use it interactively it will
#' only affect your current session. Add it to your script(s) or `.Rprofile`
#' file to avoid toggling it each time.

doesn't mention that the options should be used before load fundiversity.

Maybe it could also be good to mention when to use and not use memoisation.

@Rekyt Rekyt added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 6, 2023
@Bisaloo
Copy link
Collaborator

Bisaloo commented Nov 1, 2023

This is a good point and I wonder if it's more than a documentation issue. It seems cumbersome and error-prone to have to restart your session. Would it work & make sense to read this option in the functions calling fd_chull() and fd_chull_intersect()?

@Rekyt
Copy link
Member Author

Rekyt commented Nov 5, 2023

The problem is the way we implement memoization.
To make it work seamlessly, we check the option when loading the package.
Then we load the updated version of the functions.

fundiversity/R/zzz.R

Lines 4 to 10 in b98a6fe

.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)
}
}

Our initial goal was to make it as transparent as possible for users to use memoise. So I'm not sure implementation wise how to keep that while changing the way we check the options and load the functions.

Should we have an overarching function (like fd_chull()) that checks the option and overloads internal functions?

Something in the line of:

fd_chull <- function(...) {

  if (requireNamespace("memoise", quietly = TRUE) && 
       isTRUE(getOption("fundiversity.memoise", TRUE))) {
    fd_chull_memoised(...)
  } else {
    fd_chull_unmemoised(...)
  }
}

Rekyt pushed a commit that referenced this issue Mar 20, 2024
@Rekyt Rekyt closed this as completed in #80 Apr 3, 2024
Rekyt pushed a commit that referenced this issue Apr 3, 2024
fixes #71 and #77

---------

Co-authored-by: Hugo Gruson <[email protected]>
Co-authored-by: Matthias Grenié <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants