Multiclass support for caretList and caretStack

zachmayer / caretEnsemble

caret models all the way down :turtle:

Other

226 stars 75 forks source link

Multiclass support for caretList and caretStack #260

Open antongomez opened 2 weeks ago

antongomez commented 2 weeks ago

The following changes were made:

[x] predict.caretList now returns a matrix of probabilities for classification problems.
[x] Internally, makePredObsMatrix uses probabilities for all but one class.
[x] predict.caretStack now returns a matrix of probabilities.
[x] Functions setMulticlassExcludedLevel and getMulticlassExcludedLevel can be used to determine which class to exclude. If a class outside the range 1,...,num_classes is provided, makePredObsMatrix and predict.caretStack will use probabilities for ALL classes and a warning will be displayed, as this may cause collinearity issues.
[x] All tests have passed, and specific tests for multiclass functionality were added.

caretEnsemble still works only for binary classification problems. This is due to the method used, glm, which only works with binary class problems. Instead of this method, caretStack can be used with other methods, such as multinom, as suggested here.

zachmayer commented 2 weeks ago

Awesome! Thank you for the PR. I will review.

zachmayer commented 2 weeks ago

Sorry for the delay. I plan to review this week!

antongomez commented 1 week ago

I have a couple small comments, but it generally looks good.

Did you use an automatic linter to add all the code style stuff (e.g. spaces around "=")? In the future it's a good practice to make a PR that applies your delinting first, and then follow up with a second PR that makes your code changes.

I appreciate the delinting! It's just a good practice to separate style changes from code changes.

Thank you again! And please see PR comments for specifics changes requested.

Yes, I use an automatic linter. Initially, I tried not to use it in this project, but eventually I did and didn't realize it. Next time, I will separate it into two different PRs!

Regarding the linter test, I will remove the comments and address the warnings in another PR.

zachmayer commented 1 week ago

Awesome, thank you for the updates! I think we're almost there!

we can sort out the lint in a different PR. I actually appreciate the de-linting. I should use the same automatic tool: what do you use to lint your code as you write it?

zachmayer commented 4 days ago

@antongomez remind me where we're at here. Did you respond to all my feedback? If so I'll do one last review and merge.

(we can deal with linting in another PR once this is in)

zachmayer commented 2 days ago

Crap. I delete the branch by accident. Let me see how I can restore it

zachmayer commented 2 days ago

Ok I think I did it. Sorry. Please let me know if you need anything else before we merge this!

zachmayer commented 2 days ago

Ok, we have a lot of merge conflicts. I'll work on this.