separate InfJack from randomForestInfJack

swager / randomForestCI

This package is DEPRECATED. Please use the packages `grf` or `ranger` instead, which have built-in confidence intervals.

https://github.com/swager/grf

MIT License

69 stars 21 forks source link

separate InfJack from randomForestInfJack #2

Closed zmjones closed 8 years ago

zmjones commented 8 years ago

this should not change any functionality as you've implemented it but it would allow the estimator to be used with other compatible implementations of RF if they provide the tree prediction and inbag matrices.

i checked your examples and things appear to be working correctly. i can write some unit tests if that is desirable as well.

swager commented 8 years ago

Thanks a lot! This looks like a useful refactor. A few points:

I think it would be good to keep the "calibration" step inside the new infJack function. Without calibration, the method may produce negative variance estimates that then need to be dealt with ad-hoc. The calibration procedure is not specific to random forests.
infJack shouldn't need "newdata" as an argument; it should get everything it needs from the "pred" matrix.
If you could write some unit tests, that would be great.
For consistency, the method should be infJack instead of InfJack.

zmjones commented 8 years ago

ok thanks!

is it alright if i use testthat?

swager commented 8 years ago

testthat sounds great, thanks!

zmjones commented 8 years ago

the unit tests are a bit silly but might catch some errors. i made a few changes to some of the variables in the ij and the eb parts of the code that was originally in randomForestInfJack but I think they were all simple things. i looked at the graphs generated from your examples before and after the changes and so no obvious errors. some quick checking would be good though!

i think i incorporated all of your points. additionally i used roxygen2 to generate the namespace and added randomForest and testthat to suggests.

i assumed (due to the documentation) that you planned on exposing the EB functions as well?

cheers

swager commented 8 years ago

Thanks again for your work on this!