Skip to content

Conversation

tjpalanca
Copy link
Contributor

@tjpalanca tjpalanca commented Jun 29, 2017

This change relaxes the restrictions in makeBaseWrapper so that:

  1. Only wrappers of class TuneWrapper (instead of any of class OptWrapper) must be placed at innermost loop and cannot wrap around anything else.

This unlocks two use cases related to issues #1861 and #1806:

  1. Feature selection + hyperparameter tuning + model validation (nested-nested CV)? #1861 Allows feature selection in an outer loop and tuning within each candidate feature set.
  2. ImputationWrapper and FeatSelWrapper conflict when imputed column is missing from candidate feature set #1806 Allows imputation to take place before the feature selection loop so that it does not fail in the case when the candidate feature set in feature selection does not include specified column names via the cols argument.

Thank you!

@larskotthoff
Copy link
Member

Thanks. Could you add a test for the new checks please?

@tjpalanca
Copy link
Contributor Author

I've changed the incorrect logic in the first commit to reflect the required behavior.

I've also added two tests for: (1) the required use case mentioned in #1861 and (2) catching the error when a TuneWrapper is not in the innermost resampling loop.

@lintr-bot
Copy link

tests/testthat/test_base_BaseWrapper.R:31:41: style: Place a space before left parenthesis, except in a function call.

​      makeDiscreteParam("C", values = 2^(-2:2)),
                                        ^

tests/testthat/test_base_BaseWrapper.R:32:45: style: Place a space before left parenthesis, except in a function call.

​      makeDiscreteParam("sigma", values = 2^(-2:2))
                                            ^

tests/testthat/test_base_BaseWrapper.R:60:43: style: Place a space before left parenthesis, except in a function call.

​        makeDiscreteParam("C", values = 2^(-2:2)),
                                          ^

tests/testthat/test_base_BaseWrapper.R:61:47: style: Place a space before left parenthesis, except in a function call.

​        makeDiscreteParam("sigma", values = 2^(-2:2))
                                              ^

@larskotthoff
Copy link
Member

Thanks, this looks good to me. I'd like to have somebody else to have a look though @berndbischl @mllg @jakob-r

@larskotthoff
Copy link
Member

Ping @berndbischl @mllg @jakob-r

@larskotthoff larskotthoff merged commit 1ea4808 into mlr-org:master Jul 9, 2017
larskotthoff pushed a commit that referenced this pull request Jul 9, 2017
mllg pushed a commit that referenced this pull request Jul 11, 2017
…ng and to prevent errors in wrapped feature selection and imputation (#1871)

* Allows nested feature selection and tuning

* Only restrict tune wrappers from being wrapped

* Add tests for nested nested resampling positive case and negative case for wrapping tuning wrapper

* Fix lintrbot issues
mllg pushed a commit that referenced this pull request Jul 11, 2017
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.

4 participants