search

JuliaAI / MLJDecisionTreeInterface.jl

MIT License
9 stars 4 forks source link

Add a data front-end #41

Closed ablaom closed 1 year ago

ablaom commented 1 year ago

This PR is in support of #40.

It add the reformat/selectrows MLJModelInterface.jl data front-end. It also does a bit of cleaning up and adds generic integration tests from MLJTestInterface.jl.

The other test code changes are just adaptions of the existing tests necessary because of the data front-end addition, which changes all the fit/predict signatures.

I have also locally verified that level-4 tests from MLJTestIntegration.jl also pass.

codecov-commenter commented 1 year ago

Codecov Report

Merging #41 (39cdbbe) into dev (0f86996) will increase coverage by 10.89%. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              dev      #41       +/-   ##
===========================================
+ Coverage   86.88%   97.77%   +10.89%     
===========================================
  Files           1        1               
  Lines         122       90       -32     
===========================================
- Hits          106       88       -18     
+ Misses         16        2       -14
Impacted Files Coverage Δ
src/MLJDecisionTreeInterface.jl 97.77% <100.00%> (+10.89%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ablaom commented 1 year ago

@tylerjthomas9 Having just done this for CatBoost.jl, you will be quite familiar with the data front-end API. Any chance you have the bandwidth for a review some time in the next week or so?

tylerjthomas9 commented 1 year ago

This is not in the PR, but 10 seems like a small default value for n_trees with the random forest models. I feel like 100 is a solid default parameter, and should give better results out of the box.

Other than the comment I left, everything looks good to me. You are already working on adding a warm start method for random forest models, and I think that'll also be a great addition.

ablaom commented 1 year ago

@tylerjthomas9 Thanks for the review.

ablaom commented 1 year ago

Re default, I've posted #43