neurodata / scikit-learn

scikit-learn-tree fork: A fork that enables extensions of Python and Cython API for decision trees
https://scikit-learn.org
BSD 3-Clause "New" or "Revised" License
7 stars 6 forks source link

ENH Splitter Injection and Refactoring of DepthFirstTreeBuilder's building mechanism #67

Open SamuelCarliles3 opened 5 months ago

SamuelCarliles3 commented 5 months ago

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Includes splitter injection and adds refactor of DepthFirstTreeBuilder.build

Any other comments?

github-actions[bot] commented 5 months ago

❌ Linting issues

This PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling pre-commit hooks. Instructions to enable them can be found here.

You can see the details of the linting issues under the lint job here


cython-lint

cython-lint detected issues. Please fix them locally and push the changes. Here you can see the detected issues. Note that the installed cython-lint version is cython-lint=0.16.2.

``` /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:22:26: 'uintptr_t' imported but unused /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:23:33: 'free' imported but unused /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:76:24: E261 at least two spaces before inline comment /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:99:24: E261 at least two spaces before inline comment /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:120:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:148:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:152:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:176:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:180:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:206:53: E703 statement ends with a semicolon /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:292:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:376:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:380:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:385:5: E303 too many blank lines (2) /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:806:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:812:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:835:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:838:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_splitter.pyx:842:1: W293 blank line contains whitespace /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:273:25: E128 continuation line under-indented for visual indent /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:274:25: E128 continuation line under-indented for visual indent /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:275:25: E128 continuation line under-indented for visual indent /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:294:29: E128 continuation line under-indented for visual indent /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:295:29: E128 continuation line under-indented for visual indent /home/runner/work/scikit-learn/scikit-learn/sklearn/tree/_tree.pyx:388:1: W293 blank line contains whitespace ```

Generated for commit: f225658. Link to the linter CI: here

adam2392 commented 5 months ago

Another dumb question: why is depthfirsttreebuilder need to change, but not bestfirsttreebuilder? @SamuelCarliles3

SamuelCarliles3 commented 5 months ago

Another dumb question: why is depthfirsttreebuilder need to change, but not bestfirsttreebuilder? @SamuelCarliles3

BFTB will most certainly need to change as well, I'm just starting with DFTB, and have not yet gotten to BFTB. IIRC the update functionality had not been added to BFTB(?), and so it did not require an analogous refactor.