search

JuliaAI / DecisionTree.jl

Julia implementation of Decision Tree (CART) and Random Forest algorithms
Other
356 stars 102 forks source link

Round digits in `print_tree` #173

Closed rikhuijzer closed 2 years ago

rikhuijzer commented 2 years ago

Before (dev branch)

Feature 3, Threshold -28.166052806422238
L-> Feature 2, Threshold -161.04351901384842
    L-> 5 : 842/3650
    R-> 7 : 2493/10555
R-> Feature 7, Threshold 108.1408338577021
    L-> 2 : 2434/15287
    R-> 8 : 1227/3508

After (this PR)

Feature 3, Threshold -28.16
L-> Feature 2, Threshold -161.04
    L-> 5 : 842/3650
    R-> 7 : 2493/10555
R-> Feature 7, Threshold 108.14
    L-> 2 : 2434/15287
    R-> 8 : 1227/3508
codecov-commenter commented 2 years ago

Codecov Report

Merging #173 (3ba1651) into dev (63cb26a) will decrease coverage by 0.32%. The diff coverage is 65.00%.

@@            Coverage Diff             @@
##              dev     #173      +/-   ##
==========================================
- Coverage   89.51%   89.18%   -0.33%     
==========================================
  Files          10       10              
  Lines         992      999       +7     
==========================================
+ Hits          888      891       +3     
- Misses        104      108       +4
Impacted Files Coverage Δ
src/scikitlearnAPI.jl 39.58% <0.00%> (-0.85%) :arrow_down:
src/DecisionTree.jl 55.10% <81.25%> (+0.55%) :arrow_up:

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 63cb26a...3ba1651. Read the comment docs.

rikhuijzer commented 2 years ago

This totally makes sense. My only suggestion would be to use sigdigits instead of digits in round (and name of kwarg) because features can come with very different scales

I never knew of the existence of sigdigits. Thanks; I've updated it

ablaom commented 2 years ago

Please rebase your PR to address the conflicts. Part of these will be because I modified the print_tree doc-string before merging #172, which I did to improve the string further and resolve a separate conflict.

rikhuijzer commented 2 years ago

Don't merge yet. I should add a test first; also for #172

rikhuijzer commented 2 years ago

I've added a test in 3ba1651 and added a first argument io::IO to the print_tree signature. I figured that it would probably be best to implement that at some point anyway because it is a Julia convention, so I thought why not now.

If that feature is not acceptable, we can revert 3ba1651 and merge that. The tests do not add super much unfortunately. It's hard to test the text.