search

sylvaticus / BetaML.jl

Beta Machine Learning Toolkit
MIT License
92 stars 14 forks source link

Adapt to new `AbstractTrees.AbstractNode` type #46

Closed roland-KA closed 1 year ago

roland-KA commented 1 year ago

As there is now an abstract type AbstractNode in the AbstractTrees.jl-package, the implementation of the AbstractTrees-interface (in src/Trees/AbstractTrees.jl) could be adapted accordingly, so that it is now possible to plot a decision tree using the TreeRecipe.jl plot recipe.

There is an example on how to plot a BetaML decision tree using the recipe in TreeRecipe.jl/test/testBetaMLtrees.jl.

Note: TreeRecipe.jl isn't yet a registered package. So it has to be loaded currently from roland-KA/TreeRecipe.jl.

codecov[bot] commented 1 year ago

Codecov Report

Merging #46 (e268ce9) into master (c7bcfc1) will decrease coverage by 0.03%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   81.37%   81.33%   -0.04%     
==========================================
  Files          35       35              
  Lines        4171     4173       +2     
==========================================
  Hits         3394     3394              
- Misses        777      779       +2
Impacted Files Coverage Δ
src/Trees/AbstractTrees.jl 43.75% <0.00%> (-6.25%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

sylvaticus commented 1 year ago

Hello, I admit I didn't follow the issue very deeply, but I haven't understood the role of this unregistered package TreeRecipe.jl. Would BetaML.jl need to depend on it ? The test environment only ?

roland-KA commented 1 year ago

Applying a plot recipe instead of using a plotting package like Plots.jl directly, has the effect, that structures from a package like BetaML.jl can be visualized without the need for BetaML.jl for having dependencies to the grahpics package nor to the plot recipe (like TreeRecipe). This is a common concept which can be used in the context of Plots.jl.

So there are no dependencies to BetaML.jl introduced. Only code that wants to visualize a BetaML-decision tree needs dependencies to TreeRecipe.jl and Plots.jl (as the example in roland-KA/test/testBetaMLtrees.jl shows. It creates the following visualization: plot_3

sylvaticus commented 1 year ago

I am merging this pull request, but there is still a couple of things I can't get... (1) where the code in file /src/Trees/AbstractTrees.jl is used ? I can't see any import of that file ? (2) Where is the /test/testBetaMLtrees.jl file ? I understood that "testing" BetaML" with the plot visualisation would need TreeRecipe.jl and Plots.jl dependency. In that case we could put that test on the file Trees_tests_additional.jl that is not run by default on each push...

sylvaticus commented 1 year ago

ok, I got it.. the include call was at the bottom of the file "DecisionTrees.jl".. I renamed the file "AbstractTrees.jl" that you wrote to "AbstractTrees_BetaML_interface.jl" and moved its include to the file "Trees.jl". Should release soon with it..

roland-KA commented 1 year ago

(2) Where is the /test/testBetaMLtrees.jl file ?

Currently it's in the the package roland-KA/TreeRecipe.jl (the plot recipe), since I didn't want to interfere more with your package at the moment until things are a bit more organized.

But you are right. In the end there should be a such a test in BetaML.jl. Shall I prepare another PR to place it in Trees_tests_additional.jl?

sylvaticus commented 1 year ago

yes thanks, it would be nice to have some testing in BetaML of the BetaML code.. thank you..

roland-KA commented 1 year ago

yes thanks, it would be nice to have some testing in BetaML of the BetaML code.. thank you..

A new PR for this test is ready.