Skip to content
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

Enable multiclass classification with plsdaCaret #2621

Merged
merged 4 commits into from Jul 18, 2019

Conversation

@GegznaV
Copy link
Collaborator

commented Jul 16, 2019

Solves #2620.

Locally the new tests pass:
image

GegznaV added some commits Jul 16, 2019

@@ -36,3 +36,41 @@ test_that("classif_plsdaCaret", {
testProbParsets("classif.plsdaCaret", binaryclass.df, binaryclass.target, binaryclass.train.inds,
old.probs.list, parset.list)
})

This comment has been minimized.

Copy link
@pat-s

pat-s Jul 16, 2019

Member

Please remove the unnecessary empty lines below.

This comment has been minimized.

Copy link
@GegznaV

GegznaV Jul 16, 2019

Author Collaborator

Removed one line.

old.predicts.list, parset.list)
testProbParsets("classif.plsdaCaret", multiclass.df, multiclass.target, multiclass.train.inds,
old.probs.list, parset.list)

This comment has been minimized.

Copy link
@pat-s

pat-s Jul 16, 2019

Member

This line is also not not needed.

This comment has been minimized.

Copy link
@GegznaV

GegznaV Jul 16, 2019

Author Collaborator

Removed two lines.

@pat-s
Copy link
Member

left a comment

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?

GegznaV added some commits Jul 16, 2019

@GegznaV GegznaV requested a review from pat-s Jul 16, 2019

@GegznaV

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

I removed 3 lines and applied mlr_style. Then added a line to NEWS.md.
Still, I'm not sure if one more line should be removed?

@pat-s

pat-s approved these changes Jul 18, 2019

@pat-s

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Thanks for contributing!

@pat-s pat-s merged commit 78a96eb into mlr-org:master Jul 18, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.