JuliaAI / DecisionTree.jl

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

Float64 replaced by AbstractFloat for regression #226

Closed xinadi closed 11 months ago

xinadi commented 11 months ago

Based on #225. Occurunces of Float64 related to labels were replaced by AbstractFloat. Problem noted in https://github.com/JuliaAI/DecisionTree.jl/issues/225#issuecomment-1728241680 still not solved as requare more refactor of parameters. But origin issue solved and now other Float type than Float64 will lead to regression, not calssification. Also, I see a little perfomance boost and reducing number of allocations on my data.

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (605e4d4) 87.50% compared to head (4ccba62) 87.65%.

:exclamation: Current head 4ccba62 differs from pull request most recent head 00fd6cb. Consider uploading reports for the commit 00fd6cb to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #226 +/- ## ========================================== + Coverage 87.50% 87.65% +0.15% ========================================== Files 10 10 Lines 1328 1329 +1 ========================================== + Hits 1162 1165 +3 + Misses 166 164 -2 ``` | [Files](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI) | Coverage Δ | | |---|---|---| | [src/classification/main.jl](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI#diff-c3JjL2NsYXNzaWZpY2F0aW9uL21haW4uamw=) | `95.88% <100.00%> (-0.04%)` | :arrow_down: | | [src/measures.jl](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI#diff-c3JjL21lYXN1cmVzLmps) | `97.39% <ø> (ø)` | | | [src/regression/main.jl](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI#diff-c3JjL3JlZ3Jlc3Npb24vbWFpbi5qbA==) | `92.59% <100.00%> (+3.70%)` | :arrow_up: | | [src/regression/tree.jl](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI#diff-c3JjL3JlZ3Jlc3Npb24vdHJlZS5qbA==) | `95.07% <100.00%> (+0.03%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/JuliaAI/DecisionTree.jl/pull/226/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAI)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rikhuijzer commented 11 months ago

Thanks for opening this pull request @xinadi. I was trying to reproduce what situations this PR has fixed. Could you add a test that fails on the current version of DecisionTree.jl and succeeds on this PR?

xinadi commented 11 months ago

@rikhuijzer I added test at started form line https://github.com/JuliaAI/DecisionTree.jl/blob/605e4d41deaa547462f71b5bd05a9a16ad682b15/test/regression/low_precision.jl#L101 because float16 checks and it seems beast place for this test. The idea of test is next: if classification takes place each prediciton will be equal to the some value from training labels set because estimation based on votes. On the other hand, for regression predicitons will be means and will not be exact as from training labes set. In theory, with PR test also could be failed in the sutiation when for each predicition all the trees estimate the same value. But for 10^3 number of predictions and machine precision for mean the probability of it should be very small. Also, as I understend the seed of rng guarantees same numbers. So I see proper work of test on my PC.

rikhuijzer commented 11 months ago

Thank you @xinadi!