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

add reduceResultsList.fun arg to reduceResultsBatchmark #29

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

tdhock
Copy link
Contributor

@tdhock tdhock commented Feb 14, 2024

Hi! in my use case, I want to do model interpretation, so I am using store_models=TRUE.
But that makes my cluster system kill my R process (too much RAM used) when I do reduceResultsBatchmark, which gets killed when it calls reduceResultsList, #18 (comment) because the total of all learner_state is too large.
So in this PR I add a new argument, reduceResultsList.fun, which gets passed on, so then I can process the learner_state. For example the code below works for me using this PR. (uses reasonable amount of memory / R is not killed)

ignore.learner <- function(L){
  L$learner_state$model <- NULL
  L
}
bmr = mlr3batchmark::reduceResultsBatchmark(ids, reg = reg, store_backends = FALSE, reduceResultsList.fun=ignore.learner)

(but without the proposed reduceResultsList.fun, the above code results in my cluster system killing R)
Thanks for your consideration.

@sebffischer sebffischer requested a review from be-marc February 14, 2024 20:19
Copy link
Member

@sebffischer sebffischer left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this seems generally useful!

Please:

  1. change the function name to something shorter that does also not use a . and camelCase.
  2. Add a test
  3. Update NEWS.md and document

@tdhock
Copy link
Contributor Author

tdhock commented Feb 15, 2024

Thanks for the feedback @sebffischer is the name fun ok?

@sebffischer
Copy link
Member

Thanks for the feedback @sebffischer is the name fun ok?

How about reduce_fun ?

@tdhock
Copy link
Contributor Author

tdhock commented Feb 21, 2024

are you sure? you said it should be camelCase, and reduce_fun is not?

@sebffischer
Copy link
Member

Sorry maybe my English was imprecise. It should neither be camelCase nor contain a .

@tdhock
Copy link
Contributor Author

tdhock commented Feb 26, 2024

Hi I saw that docs are inherited from reduceResultsList, so I figured the easiest way to document the new argument would be to just give it the same name, fun, is that OK?

@sebffischer
Copy link
Member

Yes, that makes sense!

@sebffischer
Copy link
Member

Thanks again! :)

@sebffischer sebffischer merged commit 5b2b081 into mlr-org:main Apr 9, 2024
3 checks passed
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.

2 participants