search

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 enable partial_fit for forest classifiers #55

Closed PSSF23 closed 1 year ago

PSSF23 commented 1 year ago

Reference Issues/PRs

https://github.com/neurodata/scikit-tree/issues/107

What does this implement/fix? Explain your changes.

Any other comments?

github-actions[bot] commented 1 year ago

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 4f74fe4. Link to the linter CI: here

PSSF23 commented 1 year ago

@adam2392 About the set_split_node change you mentioned, is it still TODO?

adam2392 commented 1 year ago

black==23.7.0 is formatting the file differently from the linting test.

Are you talking about this: https://github.com/neurodata/scikit-learn/pull/55#issuecomment-1686731480?

It shows an issue identified via ruff on my end

adam2392 commented 1 year ago

@adam2392 About the set_split_node change you mentioned, is it still TODO?

No. I fixed it in the fork and in scikit-tree. Feel free to take a look and see if it works for the forest. If so, then all the classification forests have now streaming ability.

PSSF23 commented 1 year ago

black==23.7.0 is formatting the file differently from the linting test.

Are you talking about this: #55 (comment)?

It shows an issue identified via ruff on my end

I have modified the format to pass the linting tests, but if I do black sklearn/ensemble/_forest.py again the file will still be reformatted by black.

adam2392 commented 1 year ago

black==23.7.0 is formatting the file differently from the linting test.

Are you talking about this: #55 (comment)? It shows an issue identified via ruff on my end

I have modified the format to pass the linting tests, but if I do black sklearn/ensemble/_forest.py again the file will still be reformatted by black.

They are frozen on black == 23.3.0 rn it seems. I would just pip install pre-commit to use those tools perhaps?

adam2392 commented 1 year ago

@adam2392 About the set_split_node change you mentioned, is it still TODO?

No. I fixed it in the fork and in scikit-tree. Feel free to take a look and see if it works for the forest. If so, then all the classification forests have now streaming ability.

Idk if high-priority, but it would be really cool to see how streaming works wrt ObliqueRF and MORF. Lmk if you want to discuss.