[ENH v2] Add partial fit to the correct branch for decisiontreeclassifier

[adam2392]

Supersedes: #50

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

[github-actions]

✔️ Linting Passed

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

_{Generated for commit: 368df7a. Link to the linter CI: here}

[adam2392]

@PSSF23 does this code affect the normal building process not via partial_fit

[PSSF23]

it shouldn't. update_stack only exists when the initial_roots has content.

[adam2392]

@PSSF23

The CI errors seem related to the code introduced: https://dev.azure.com/neurodata/neurodata/_build/results?buildId=590&view=logs&j=dde5042c-7464-5d47-9507-31bdd2ee0a3a&t=4bd2dad8-62b3-5bf9-08a5-a9880c530c94

[PSSF23]

@adam2392 Yes, the classes param was not properly passed in by the CI, so it was expecting a different kind of error. We can remove the _check_partial_fit_first_call check, but then there might be other errors associated with partial_fit api.

By the way, the performance is improved with the new branch, about the same as streaming tree on its own now. So previously the inferior results might be due to some bug in submodulev2.

[adam2392]

@adam2392 Yes, the classes param was not properly passed in by the CI, so it was expecting a different kind of error. We can remove the _check_partial_fit_first_call check, but then there might be other errors associated with partial_fit api.

Can you elaborate? Perhaps we can move the check to a better place and then have everything pass?

[PSSF23]

It appears that they are testing all methods with fitting functions (fit, partial_fit, ...) about ccp_alpha, but did not give partial_fit the classes variable it requires. As long as _check_partial_fit_first_call exists there will be errors, because partial_fit could not run properly without classes. It was added because it was a requirement for the partial_fit api.

The checks for ccp_alpha, which they are testing, exist in _build_pruned_tree_ccp. So it can only be tested correctly after there's a tree.

[PSSF23]

For unit tests, sklearn had built-in checks for partial_fit from other streaming algorithms, so I didn't prepare any extra ones. We can add new ones as we see fit, but the existing ones should cover most situations.

[adam2392]

For unit tests, sklearn had built-in checks for partial_fit from other streaming algorithms, so I didn't prepare any extra ones. We can add new ones as we see fit, but the existing ones should cover most situations.

I see. I took a look and all you have to do is validate the input parameters before running the partial fit check: 63637f7

[adam2392]

Do you want to add the support for partial_fit into ForestClassifier?

[PSSF23]

@adam2392 Thanks for all the improvements. I need to work on simulations for the cancer project this week so feel free to go ahead. We also found some potential bugs on honest code so I might need to revisit it.

[adam2392]

Kay merged! @PSSF23 feel free to open up a PR to enable this for the ForestClassifier class.