Brulee neural networks with two hidden layers #1187
Conversation
This reverts commit bec0423.
|
Leaving a note for both of us that I'm waiting with review until the tests are in extratests and you've had a chance to go through the "Add a new parsnip model/engine" checklist of tidymodels/tidymodels#97 👍 |
There was a problem hiding this comment.
Hello and welcome new engine! 😍 I have two outstanding items from the checklist that I'd like to see included and I am making the case for sticking with established code patterns, given the size of parsnip. Other than that only a couple of minor things down to nits for readability.
The one failing GHA is a failing test on speed which we should look into but not as part of this PR - I don't see how adding this engine would have caused this or how we would speed up the engine registration in this PR.
From the checklist:
- If the engine is added in parsnip, update
vignettes/articles/Examples.Rmd. If the engine is added in an extension package: is there a corresponding place that needs updating (e.g., does the README list available models/engines)? - Add a documentation entry in NEWS.md
| @@ -0,0 +1,11 @@ | |||
| #' Multilayer perceptron via brulee with two hidden layers | |||
| #' | |||
| #' [brulee::brulee_mlp_two_layer()] fits a neural network (with version 0.3.0.9000 or higher of brulee) | |||
There was a problem hiding this comment.
| #' [brulee::brulee_mlp_two_layer()] fits a neural network (with version 0.3.0.9000 or higher of brulee) | |
| #' [brulee::brulee_mlp_two_layer()] fits a neural network |
This is going to look outdated very soon. Let's only specify the version requirement in code.
There was a problem hiding this comment.
Shouldn't brulee be in Suggests in the DESCRIPTION? 🤔
|
|
||
| set_model_engine("mlp", "classification", "brulee_two_layer") | ||
| set_model_engine("mlp", "regression", "brulee_two_layer") | ||
| set_dependency("mlp", "brulee_two_layer", "brulee") |
There was a problem hiding this comment.
This is missing the specification of the mode.
| set_model_arg( | ||
| model = "mlp", | ||
| eng = "brulee_two_layer", | ||
| parsnip = "epochs", | ||
| original = "epochs", | ||
| func = list(pkg = "dials", fun = "epochs"), | ||
| has_submodel = FALSE | ||
| ) | ||
|
|
||
| set_model_arg( | ||
| model = "mlp", | ||
| eng = "brulee_two_layer", | ||
| parsnip = "dropout", | ||
| original = "dropout", | ||
| func = list(pkg = "dials", fun = "dropout"), | ||
| has_submodel = FALSE | ||
| ) |
There was a problem hiding this comment.
nit: can we list them in the same order as they are arguments to mlp()? that would mean first dropout then epochs.
| set_model_arg( | ||
| model = "mlp", | ||
| eng = "brulee_two_layer", | ||
| parsnip = "learn_rate", | ||
| original = "learn_rate", | ||
| func = list(pkg = "dials", fun = "learn_rate", range = c(-2.5, -0.5)), | ||
| has_submodel = FALSE | ||
| ) | ||
|
|
||
| set_model_arg( | ||
| model = "mlp", | ||
| eng = "brulee_two_layer", | ||
| parsnip = "activation", | ||
| original = "activation", | ||
| func = list(pkg = "dials", fun = "activation", values = c('relu', 'elu', 'tanh')), | ||
| has_submodel = FALSE | ||
| ) |
There was a problem hiding this comment.
same nit here, first activation then learn_rate
|
|
||
| Parsnip changes the default range for `learn_rate` to `c(-2.5, -0.5)`. | ||
|
|
||
| - `rate_schedule()`: A function to change the learning rate over epochs. See [brulee::schedule_decay_time()] for details. |
There was a problem hiding this comment.
Why do the argument names have parentheses after them? That makes these look like functions. If that's intentional, then let's explain why. Otherwise, let's remove the parentheses.
| translate() | ||
| ``` | ||
|
|
||
| Note that parsnip automatically sets linear activation in the last layer. |
There was a problem hiding this comment.
| Note that parsnip automatically sets linear activation in the last layer. | |
| Note that parsnip automatically sets the linear activation in the last layer. |
| @@ -246,34 +245,19 @@ tunable.linear_reg <- function(x, ...) { | |||
| res$call_info[res$name == "mixture"] <- | |||
| list(list(pkg = "dials", fun = "mixture", range = c(0.05, 1.00))) | |||
| } else if (x$engine == "brulee") { | |||
| res <- add_engine_parameters(res, brulee_linear_engine_args) | |||
There was a problem hiding this comment.
Can we stick with this established pattern of using add_engine_parameters()?
| } | ||
| res | ||
| } | ||
| tunable.logistic_reg <- tunable.linear_reg |
There was a problem hiding this comment.
Can we keep the separate definitions of tunable() methods per model type? It is a bit of code duplication but allows us to add tunable engine arguments for different model types independently. I think that modularisation is worth the duplication.
| list(list(pkg = "dials", fun = "learn_rate", range = c(-3, -1/2))) | ||
| res$call_info[res$name == "epochs"] <- | ||
| list(list(pkg = "dials", fun = "epochs", range = c(5L, 500L))) | ||
| if (grepl("brulee", x$engine)) { |
There was a problem hiding this comment.
Here again, I'd argue for sticking with the pattern, given how big parsnip is by now. If we think a different pattern is better, we should do that in a separate issue-PR pair for all models/engines.
| } else if (any(names(x) == "values")) { | ||
| cl <- rlang::call2(x$fun, .ns = x$pkg, values = x$values) |
There was a problem hiding this comment.
This is reaching the length where it might be nice to just splice remaining components into the call but I'm also okay with this staying as is.
A complement to tidymodels/brulee#84 and the previous tidymodels/brulee#80
Tests will be in extratests