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

Adding minNode functionality of 'ranger' and 'Rborist' #732

Merged
merged 3 commits into from Sep 25, 2017

Conversation

@hadjipantelis
Copy link
Contributor

@hadjipantelis hadjipantelis commented Sep 7, 2017

Both 'ranger' and 'Rborist' offer the option to control the minimum node size. This is convenient to control under/over-fitting in a natural way. All test in RegressionTests/Code work fine.

Side note: One of the test_reg_rand folds in Rborist.R produced a warning because when splitting a very small sample for CV one of the folds is too small and Rborist erred - this run produced an NA but I think it is a "good thing"; it is irrational to use very strong regularisation with very small training sets (<= 20).

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 7, 2017

Codecov Report

Merging #732 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #732      +/-   ##
==========================================
- Coverage   16.97%   16.97%   -0.01%     
==========================================
  Files          90       90              
  Lines       13185    13187       +2     
==========================================
  Hits         2238     2238              
- Misses      10947    10949       +2
Impacted Files Coverage Δ
R/knn3.R 57.74% <0%> (-0.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb1c6a7...edd95f2. Read the comment docs.

@topepo
Copy link
Owner

@topepo topepo commented Sep 22, 2017

Looks good. Can you add an exit to the NEWS.md to document the changes?

@hadjipantelis
Copy link
Contributor Author

@hadjipantelis hadjipantelis commented Sep 23, 2017

Cool. Yes; NEWS.md updated.

Side-note: NEWS.md does not contain any updates about the (re-)inclusion of the mxnet* models. Should it be mentioned too?

@topepo
Copy link
Owner

@topepo topepo commented Sep 25, 2017

Side-note: NEWS.md does not contain any updates about the (re-)inclusion of the mxnet* models. Should it be mentioned too?

Yes, please. I got those in at the end and didn't document it.

@hadjipantelis
Copy link
Contributor Author

@hadjipantelis hadjipantelis commented Sep 25, 2017

No problem; done. Also added some missing ..

@topepo topepo merged commit 7540594 into topepo:master Sep 25, 2017
2 of 3 checks passed
2 of 3 checks passed
codecov/project 16.97% (-0.01%) compared to eb1c6a7
Details
codecov/patch Coverage not affected when comparing eb1c6a7...edd95f2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@topepo
Copy link
Owner

@topepo topepo commented Sep 25, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.