-
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
Conversation
I like the way you designed it! I have however a question, because, as I understood the way What I envisioned initially was to create memoized version of
|
I'm coming back with some data to support my understanding. # A tibble: 2 × 13
expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
<bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
1 regular_no_memoise 1.16s 1.24s 0.811 26MB 7.30 5 45 6.17s <list [50]> <Rprofmem [21,217 × 3]> <bench_tm> <tibble>
2 regular_memoised 1.07s 1.27s 0.818 32.5MB 12.8 3 47 3.67s <list [50]> <Rprofmem [25,040 × 3]> <bench_tm> <tibble> Compared to the solution that creates a memoised version of chull when loading the package with: .onLoad <- function(libname, pkgname) {
fd_chull_memoised <<- memoise::memoise(fd_chull)
} and f <- if (use_memoise()) {
fd_chull_memoised
} else {
fd_chull
} These are the results I'm getting: > bench::mark(
+ onload_no_memoise = {
+ options(fundiversity.memoise = FALSE)
+
+ lapply(1:50, \(x) fd_fric(traits_birds, site_sp_birds))
+ },
+ onload_memoised = {
+ options(fundiversity.memoise = TRUE)
+
+ lapply(1:50, \(x) fd_fric(traits_birds, site_sp_birds))
+ }, iterations = 50)
# A tibble: 2 × 13
expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
<bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
1 onload_no_memoise 1.12s 2.26s 0.492 26.1MB 3.02 7 43 14.2s <list [50]> <Rprofmem [28,169 × 3]> <bench_tm [50]> <tibble>
2 onload_memoised 730.43ms 792.26ms 1.26 20.5MB 1.74 21 29 16.7s <list [50]> <Rprofmem [15,607 × 3]> <bench_tm [50]> <tibble> So, clearly memoizing I would rather go for the former because I think it may be more useful, but this depends on how people are using fundiversity. |
Yes, I think you're right and what you're proposing makes sense. I can implement it next week if that sounds good to you. I also plan to address #71 (comment) in this PR, as mentioned in the first comment. |
Sounds great! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 11 +1
Lines 269 295 +26
=========================================
+ Hits 269 295 +26 ☔ View full report in Codecov by Sentry. |
Hey @Bisaloo, I finally took the time to work on this PR. I have two main issues with it for now:
Please tell me if you would have the time to review the PR |
One thing to do before merging is to bump the minor version number and update the NEWS file accordingly. I also wonder if we should document memoization elsewhere in the package. |
if (!inherits(future::plan(), "sequential")) { | ||
return(FALSE) | ||
} |
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 at FALSE
and then is set to TRUE
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.
Co-authored-by: Hugo Gruson <[email protected]>
Co-authored-by: Hugo Gruson <[email protected]>
…ndiversity into memoise-on-the-fly
This is a test to fix #77. I seem to remember we tried a similar approach initially but encountered some issues. I don't remember what exactly so this PR will at least serve the purpose of documenting our design choice better.
To do:
f
use_memoise()