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

Update variance estimation methods for RF learners #1784

Merged
merged 18 commits into from May 26, 2019

Conversation

Projects
None yet
6 participants
@ja-thomas
Copy link
Member

commented Apr 26, 2017

  • Extratrees is now an available splitting method for classification and regression

  • Add se estimation method for regr.ranger. (This is mostly copy paste code from regr.randomForest) and only one of the three alternatives there.

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

isnt it a bit inconsitent to add only 1 se method from rf...?

yes, it is a clear improvement. can you easily convince me that we should not copy the other 2 methods?

@ja-thomas

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

yup you're right, all three methods would make sense.

But I just needed one for now to test some stuff for MBO. But in general it would make sense if the se estimations for random forest would work for all supported RFs, but thats more work.

So the short answer is I was too lazy to copy all three methods... And this is mostly a PR so that the code/branch does not get lost

@jakob-r

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

We should really have extra methods for se calculation for bagged learners...

@SteveBronder

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2017

What else needs done here? May have time next week to help if you need any

@jakob-r

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

@berndbischl We should merge and be okay with this double code because calculating the SE for bagged learners in general would be a seperate issue. This should not hold back such important progress for the usage of ranger.
In other words: The benefit of being able to calculate SE for ranger outweighs the disadvantage of the little code clutter in the inside. Especially as the latter can be easily fixed later.

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2017

i really dont want the package to become inconsistent here.
if people want to have this so urgently, please help making this better.

it is really not that hard here.

@jakob-r

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

Okay. I added generalized functions for sd and jackknife. We might want to put them in a separate file. For a better diff i will do that later (name suggestions are welcome).

jakob-r added some commits Jun 23, 2017

@jakob-r

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

@berndbischl your opinion would be much appreciated 😉

@larskotthoff

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I've tried to sort out the merge conflicts. Could someone please have a look whether this still makes sense? @jakob-r

@jakob-r

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

I solved the conflict. If travis passes this can be merged.

Lars Kotthoff added some commits Dec 14, 2017

Lars Kotthoff
Lars Kotthoff
...
@larskotthoff

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

Ok, I've fixed some obvious stuff, but Travis is still failing and have no idea why. Can't reproduce locally.

@berndbischl berndbischl force-pushed the master branch from 50a547b to cf3db65 Mar 3, 2018

@berndbischl berndbischl force-pushed the master branch from cf3db65 to 57f099e Mar 3, 2018

@ja-thomas ja-thomas force-pushed the master branch from 57f099e to 20d5a39 Mar 3, 2018

merge master
Merge branch 'master' into ranger_sd_estim

# Conflicts:
#	R/RLearner_regr_ranger.R
@pat-s

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@ja-thomas just FYI, I merged the master branch. Let's see if the build passes. Do you still think that this is ready to merge then?

(+ an entry to NEWS.md)

@pat-s

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@jakob-r
For some reason the par.vals list in

learner = makeLearner("regr.ranger", predict.type = "se", par.vals = par.vals)

is not present during the train() call in

tn = getTaskTargetNames(.task)

That is what causes Travis to fail.

Do you know if this is a general problem of par.vals not being forwarded?

@jakob-r jakob-r self-assigned this May 24, 2019

jakob-r added some commits May 24, 2019

Merge branch 'master' into ranger_sd_estim
# Conflicts:
#	R/RLearner_regr_randomForest.R
#	R/RLearner_regr_ranger.R
@jakob-r

This comment has been minimized.

Copy link
Member

commented May 24, 2019

ranger now uses only its own se methods (jacknife and infjacknife (default)).
Normal rf should work again.
all necessary parameters are set automatically if predict.type = "se" is demanded.

From my side this should be mergable if no errors occur (already checked locally)

@pat-s

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Cool 👍
The doc still needs to be transformed to markdown and we a NEWS entry, then we're good to go here.

pat-s added some commits May 26, 2019

@pat-s pat-s changed the title Add extratrees and variance estimation to ranger Update variance estimation methods for RF learners May 26, 2019

@pat-s pat-s merged commit 9e2a35c into master May 26, 2019

1 check passed

deploy/netlify Deploy preview ready!
Details

@pat-s pat-s deleted the ranger_sd_estim branch May 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.