search

mlr-org / mlr3proba

Probabilistic Learning for mlr3
https://mlr3proba.mlr-org.com/
GNU Lesser General Public License v3.0
128 stars 20 forks source link

How to deal with learners that require attached namespace? #62

Closed RaphaelS1 closed 4 years ago

RaphaelS1 commented 4 years ago

For example gamboost and mboost will throw an error if either the library mboost is not attached or if the functions bbs,bols,btree are not imported. There are three solutions:

  1. Import bbs,bols,btree and add mboost to imports
  2. Add a message to the learner description telling users to attach the namespace
  3. Attach the namespace in the call to the learner

Is there an mlr/mlr3 convention for this?

RaphaelS1 commented 4 years ago

@mllg if you have any suggestions I can include this in the next patch

mllg commented 4 years ago
  1. Create a bug report upstream.
  2. I use withr to temporarily attach the package and then detach it again. See https://github.com/mlr-org/mlr3learners/blob/master/R/LearnerClassifKKNN.R#L62.
  3. I just noticed that withr always detaches the package, even if it was attached before. This is probably not what we want. I can provide a custom helper in mlr3misc to deal with this w/o changing the global state of the R session (althouhg detaching a package is always somewhat fragile in R).
mllg commented 4 years ago

For the third point there already is a bug report: https://github.com/r-lib/withr/issues/107. Unsure if they fix it.

RaphaelS1 commented 4 years ago

Ah so it is genuinely a bug, okay I'll add an issue, thanks

mllg commented 4 years ago

Added a helper to mlr3misc in https://github.com/mlr-org/mlr3misc/commit/1e5c4e2b2fb39e5286417f8eb53f89bd9a1d3dc2, will upload mlr3misc to CRAN now.

RaphaelS1 commented 4 years ago

Great! This looks like withr but without the detach bug? Thank you!!

fkiraly commented 4 years ago

This may be a stupid question, @mllg, but if you fixed the bug (where considering it as such may perhaps be a matter of taste), why don't you PR withr? Even if your code is better, do you really want to put it on your already huge pile of things to maintain?

mllg commented 4 years ago

I usually try to "repair" stuff upstream and send PRs, but after reading https://github.com/r-lib/withr/issues/107, I don't have the impression upstream is interested in such a PR.

If withr gets fixed (the required change is trivial), I will just deprecate and then remove the helper function from mlr3misc again.

fkiraly commented 4 years ago

... you can also always make a new upstream when it goes stale?