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

Added Confusionmatrix function #1798

Merged
merged 29 commits into from May 22, 2019

Conversation

Projects
None yet
4 participants
@jeffin143
Copy link
Contributor

commented Mar 20, 2019

I have implemented Confusion Matrix function along with test.A confusion matrix is a technique for summarizing the performance of a classification algorithm.

Though there are function to calculate precision and recall still calculating a confusion matrix can give you a better idea of what your classification model is getting right and what types of errors it is making

jeffin143 added some commits Mar 20, 2019

Show resolved Hide resolved src/mlpack/core/data/cm_impl.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/cm.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/cm.hpp Outdated

jeffin143 added some commits Mar 22, 2019

Show resolved Hide resolved src/mlpack/core.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp Outdated
@zoq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@jeffin143 For future commits, please use more descriptive commit messages, there might be a time where someone likes to track down a certain modification and a good commit message might be helpful here.

Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp
Show resolved Hide resolved src/mlpack/tests/cv_test.cpp Outdated
Show resolved Hide resolved src/mlpack/tests/cv_test.cpp Outdated
@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@jeffin143 For future commits, please use more descriptive commit messages, there might be a time where someone likes to track down a certain modification and a good commit message might be helpful here.

@zoq , Sorry for the bad nomenclature.

@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@zoq , Is this PR really needed, I guess Confusion matrix is good, but graphically more good, just like python plotting it and all, But as of here it is just output.at(predictors[i], responses[i])++; inside a for loop which a user can also do, if he really wants.

Would like to hear from you.

cc @rcurtin

jeffin143 added some commits Apr 26, 2019

@zoq

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

I agree the implementation is really simple, but I don't mind to have the support; unless you or anyone else likes to close this one, I think we can finish the work on this one.

@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

I agree the implementation is really simple, but I don't mind to have the support; unless you or anyone else likes to close this one, I think we can finish the work on this one.

Yup, I did raise the doubt because of simple implementation. Umm, May be removing it either wouldn't effect the build time to a great extent. May be it could be useful, to just call confusionmatrix() for user rather than to write the code for it. So let's keep this open.

I have incorporated the changes mentioned by you, The pr is read for review.

Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix_impl.hpp Outdated

jeffin143 added some commits May 9, 2019

@zoq

zoq approved these changes May 12, 2019

Copy link
Member

left a comment

Looks good to me, just left some minor style comments.

Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp Outdated
@rcurtin
Copy link
Member

left a comment

Not sure why @mlpack-bot didn't do its job here... anyway, looks good to me. Just a few style issues and I can fix them during merge (which I'll do now). 👍 I'll also update HISTORY.md.

Show resolved Hide resolved src/mlpack/core.hpp
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp
Show resolved Hide resolved src/mlpack/core/data/confusion_matrix.hpp

@rcurtin rcurtin merged commit 5500647 into mlpack:master May 22, 2019

6 checks passed

LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Not sure why @mlpack-bot didn't do its job here... anyway, looks good to me. Just a few style issues and I can fix them during merge (which I'll do now). I'll also update HISTORY.md.

Thanks :)

@jeffin143 jeffin143 deleted the jeffin143:confusionmatrix branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.