-
-
Notifications
You must be signed in to change notification settings - Fork 405
Enable multiclass classification with plsdaCaret #2621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -36,3 +36,41 @@ test_that("classif_plsdaCaret", { | |||
testProbParsets("classif.plsdaCaret", binaryclass.df, binaryclass.target, binaryclass.train.inds, | |||
old.probs.list, parset.list) | |||
}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the unnecessary empty lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed one line.
old.predicts.list, parset.list) | ||
testProbParsets("classif.plsdaCaret", multiclass.df, multiclass.target, multiclass.train.inds, | ||
old.probs.list, parset.list) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is also not not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I haven't used plsda myself yet so I am not aware if there are theoretical issues fir multiclass tasks.
If tests are working, I see no point why we should not add the multiclass property.
Providing a screenshot of your local tests is nice but not really needed since Travis will do the work :)
Could you add an entry to NEWS.md?
I removed 3 lines and applied |
Thanks for contributing! |
* Property "multiclass" added to learner plsdaCaret * Test for multiclass classif_plsdaCaret * Empty lines removed * Update NEWS.md
Solves #2620.
Locally the new tests pass:
