JuliaAI / DecisionTree.jl

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

Improved label printing/plotting of nodes/leaves #200

Closed roland-KA closed 1 year ago

roland-KA commented 1 year ago
codecov-commenter commented 1 year ago

Codecov Report

Merging #200 (d7fccfa) into dev (a072539) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #200      +/-   ##
==========================================
+ Coverage   87.97%   87.99%   +0.01%     
==========================================
  Files          10       10              
  Lines        1247     1249       +2     
==========================================
+ Hits         1097     1099       +2     
  Misses        150      150              
Impacted Files Coverage Δ
src/abstract_trees.jl 91.30% <100.00%> (+0.82%) :arrow_up:

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

roland-KA commented 1 year ago

With this PR, the sigdigits keyword argument can not be modified by the user, correct? It is called from AbstractTrees.print_tree, but the default interface for AbstractTrees.printnode has no sigdigits keyword argument, so that will not be used.

That's correct.

Should we perhaps call AbstractTrees.print_tree(f::Function, io::IO, tree; kwargs...) where f is a custom implementation of printnode with signature f(io::IO, node) to fix this? I don't see yet how, but maybe you or Anthony does. I'm having a hard time with all the print logic and figuring out what is called when.

I've also had some thoughts on how to make sigdigits modifiable by the user. To make it work with print_tree, your suggestion is the right way to go. But our actual goal is to use it in the plot recipe (where I use printnode directly). And there I have to do some rework on several functions.

In any way, I would say this PR is good to go with the default keyword argument so that the issue with TreeRecipe.jl can be fixed (#197). Probably it would be best to make it a breaking release to ensure that we're not accidentally breaking someone's code.

Because of the rework needed in TreeRecipe.jl, I agree with you, that we should merge this PR as is and postpone the rest to another update.

As TreeRecipe.jl isn't a registered package yet, I don't think that there are any users out there whose code we could brake. But if you feel safer, just make it a breaking release.

And thank's for the improvements you suggested! 👍