JuliaAI / MLJDecisionTreeInterface.jl

MIT License
9 stars 4 forks source link

Update doc-strings to meet new standard #12

Closed ablaom closed 2 years ago

ablaom commented 2 years ago

This PR needs:

This PR is simultaneously a genuine draft PR to revise the DecisionTree document strings, and a live proposal of what the [new standard]() for doc-strings should look like. General discussion of the standard should go [here](). However, specific line-by-line comments can also be made in this PR in the usual way.

ablaom commented 2 years ago

@bkamins Am responding to your comment here.

There has already been some discussion about what features are accepted by DecisionTree.jl: See https://github.com/JuliaAI/MLJDecisionTreeInterface.jl/issues/10 and https://github.com/bensadeghi/DecisionTree.jl/issues/92 . I believe the statement in DecisionTree.jl readme does have a bug - which I just reported here. What I believe is accurate is that DecisionTree accepts any feature for which < is defined. For the MLJ wrapper, this translates into any Count, OrderedFactor or Continuous feature, but not Multiclass features, which is what we have in the current PR.

I couldn't find any mention of missing value support in the DecisionTree.jl readme - could you point this out to me? I doubt features can have missing values, as this breaks ordering (the BetaML tree models do support missing however). I imagine missing values in the target are possibly allowed, but I can't find this documented.

bkamins commented 2 years ago

I couldn't find any mention of missing value support in the DecisionTree.jl readme - could you point this out to me?

I just manually checked that they were allowed in target - i.e. the model was built without error (but I do not know if then they are dropped or considered as a separate class)

codecov-commenter commented 2 years ago

Codecov Report

Merging #12 (9a3f54c) into master (ab1e98e) will increase coverage by 0.19%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   83.52%   83.72%   +0.19%     
==========================================
  Files           1        1              
  Lines          85       86       +1     
==========================================
+ Hits           71       72       +1     
  Misses         14       14              
Impacted Files Coverage Δ
src/MLJDecisionTreeInterface.jl 83.72% <100.00%> (+0.19%) :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 ab1e98e...9a3f54c. Read the comment docs.