-
Notifications
You must be signed in to change notification settings - Fork 90
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
A complement to tidymodels/brulee#84 and the previous tidymodels/brulee#80
Tests will be in extratests