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

Add precision, recall and F1 for the cross-validation and hyper-parameter tuning modules #1074

Merged
merged 11 commits into from Aug 10, 2017

Conversation

Projects
None yet
2 participants
@micyril
Member

micyril commented Aug 1, 2017

No description provided.

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 1, 2017

Member

For F1 it is unclear for me what we should return if precision and recall turn out to be zeros. I see the following options.

  1. To return NaN (the result of 2 * 0.0 * 0.0 / (0.0 + 0.0))
  2. To return 0.0 (since F1 should serve as a harmonic mean of precision and recall)
  3. To throw an exception.
Member

micyril commented Aug 1, 2017

For F1 it is unclear for me what we should return if precision and recall turn out to be zeros. I see the following options.

  1. To return NaN (the result of 2 * 0.0 * 0.0 / (0.0 + 0.0))
  2. To return 0.0 (since F1 should serve as a harmonic mean of precision and recall)
  3. To throw an exception.
@rcurtin

Looks good to me. Only a few very minor comments. Thanks for the hard work!

Show outdated Hide outdated src/mlpack/core/cv/metrics/precision.hpp
* 1. Micro. If there are N + 1 classes in total, the result is equal to
* (tp0 + tp1 + ... + tpN) / (tp0 + tp1 + ... + tpN + fp0 + fp1 + ... fpN),
* where tpI and fpI are the numbers of true positives and false positives
* respectively for the class (label) I.

This comment has been minimized.

@rcurtin

rcurtin Aug 1, 2017

Member

You can use inline LaTeX here with Doxygen, it would probably look nicer in the resulting generated documentation:

an inline formula is @f$ x + y @f$, and a standalone one is

@f[
(tp_0 + tp_1 + \ldots + tp_N) / (\ldots)
@f]

I think this comment would apply a few other places too.

@rcurtin

rcurtin Aug 1, 2017

Member

You can use inline LaTeX here with Doxygen, it would probably look nicer in the resulting generated documentation:

an inline formula is @f$ x + y @f$, and a standalone one is

@f[
(tp_0 + tp_1 + \ldots + tp_N) / (\ldots)
@f]

I think this comment would apply a few other places too.

Show outdated Hide outdated src/mlpack/core/cv/metrics/precision_impl.hpp
size_t tpTotal = 0;
for (size_t c = 0; c < numClasses; ++c)
tpTotal += arma::sum((labels == c) % (predictedLabels == c));

This comment has been minimized.

@rcurtin

rcurtin Aug 1, 2017

Member

Could you do arma::sum(labels == predictedLabels) here and drop the for loop?

@rcurtin

rcurtin Aug 1, 2017

Member

Could you do arma::sum(labels == predictedLabels) here and drop the for loop?

This comment has been minimized.

@micyril

micyril Aug 2, 2017

Member

A nice catch. I guess we can just reuse Accuracy.

@micyril

micyril Aug 2, 2017

Member

A nice catch. I guess we can just reuse Accuracy.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 2, 2017

Member

Ah sorry I realize I didn't answer the question about F1. I believe returning 0 would be the best option in this case; this is what a user is most likely to expect (something in the range of 0 to 1).

Another couple questions---

  1. Now the Micro averaging strategy is just Accuracy in all cases that you are adding here. Is there a reason to keep the Micro enum then, or should users simply use Accuracy directly?

  2. Should the binary strategy allow specifying a different target class than class 0? This could provide some extra flexibility for users.

Member

rcurtin commented Aug 2, 2017

Ah sorry I realize I didn't answer the question about F1. I believe returning 0 would be the best option in this case; this is what a user is most likely to expect (something in the range of 0 to 1).

Another couple questions---

  1. Now the Micro averaging strategy is just Accuracy in all cases that you are adding here. Is there a reason to keep the Micro enum then, or should users simply use Accuracy directly?

  2. Should the binary strategy allow specifying a different target class than class 0? This could provide some extra flexibility for users.

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 2, 2017

Member

Now the Micro averaging strategy is just Accuracy in all cases that you are adding here. Is there a reason to keep the Micro enum then, or should users simply use Accuracy directly?

Maybe some users don't know that microaveraged precision, recall and F1 are the same as accuracy (It was the case for me before I started to write the code). You can also see that some other libraries, like scikit-learn, keep the microaveraged metrics along with accuracy.

Should the binary strategy allow specifying a different target class than class 0? This could provide some extra flexibility for users.

I can imagine the following interface:

static const size_t targetClass = 5;
double precision = Precision<Binary, targetClass>::Evaluate(model, data, labels);

What do you think?

Member

micyril commented Aug 2, 2017

Now the Micro averaging strategy is just Accuracy in all cases that you are adding here. Is there a reason to keep the Micro enum then, or should users simply use Accuracy directly?

Maybe some users don't know that microaveraged precision, recall and F1 are the same as accuracy (It was the case for me before I started to write the code). You can also see that some other libraries, like scikit-learn, keep the microaveraged metrics along with accuracy.

Should the binary strategy allow specifying a different target class than class 0? This could provide some extra flexibility for users.

I can imagine the following interface:

static const size_t targetClass = 5;
double precision = Precision<Binary, targetClass>::Evaluate(model, data, labels);

What do you think?

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 2, 2017

Member

Maybe some users don't know that microaveraged precision, recall and F1 are the same as accuracy (It was the case for me before I started to write the code). You can also see that some other libraries, like scikit-learn, keep the microaveraged metrics along with accuracy.

That seems fine to me, not a bad idea to keep an alias around like this.

I think the interface you proposed is good, it's the idea I had in mind too.

Member

rcurtin commented Aug 2, 2017

Maybe some users don't know that microaveraged precision, recall and F1 are the same as accuracy (It was the case for me before I started to write the code). You can also see that some other libraries, like scikit-learn, keep the microaveraged metrics along with accuracy.

That seems fine to me, not a bad idea to keep an alias around like this.

I think the interface you proposed is good, it's the idea I had in mind too.

@rcurtin

rcurtin approved these changes Aug 5, 2017

Looks good to me. Let's let this sit for 3 days before merge in case anyone else has any comments.

Show outdated Hide outdated src/mlpack/core/cv/metrics/f1.hpp
*/
template<AverageStrategy AS>
template<AverageStrategy AS, size_t PositiveClass = 1>

This comment has been minimized.

@rcurtin

rcurtin Aug 5, 2017

Member

I'd suggest a default positive label of 0 since that's the smallest possible label.

@rcurtin

rcurtin Aug 5, 2017

Member

I'd suggest a default positive label of 0 since that's the smallest possible label.

typename = void>
static double Evaluate(MLAlgorithm& model,
const DataType& data,
const arma::Row<size_t>& labels);

This comment has been minimized.

@rcurtin

rcurtin Aug 5, 2017

Member

Wow, the refactoring necessary to allow the user to specify a positive class is a lot harder than I thought! I think that you might be able to use SFINAE in the arguments instead of adding the extra typename = voids:

template<AverageStrategy _AS, typename MLAlgorithm, typename DataType>
static double Evaluate(
    MLAlgorithm& model,
    const DataType& data,
    const arma::Row<size_t>& labels,
    const std::enable_if_t<_AS == Micro>* = 0);

and likewise for _AS == Macro and _AS == Binary.

@rcurtin

rcurtin Aug 5, 2017

Member

Wow, the refactoring necessary to allow the user to specify a positive class is a lot harder than I thought! I think that you might be able to use SFINAE in the arguments instead of adding the extra typename = voids:

template<AverageStrategy _AS, typename MLAlgorithm, typename DataType>
static double Evaluate(
    MLAlgorithm& model,
    const DataType& data,
    const arma::Row<size_t>& labels,
    const std::enable_if_t<_AS == Micro>* = 0);

and likewise for _AS == Macro and _AS == Binary.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 9, 2017

Member

@micyril: do you want to work out the merge conflict? If so I will merge this. Also, do you want to change the default positive label to 0?

Member

rcurtin commented Aug 9, 2017

@micyril: do you want to work out the merge conflict? If so I will merge this. Also, do you want to change the default positive label to 0?

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 9, 2017

Member

Ok

Member

micyril commented Aug 9, 2017

Ok

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 9, 2017

Member

Are you sure you want to see 0 as the default positive label? For me it is more natural to have 1 for that. In scikit-learn also 1 is used as default.

Member

micyril commented Aug 9, 2017

Are you sure you want to see 0 as the default positive label? For me it is more natural to have 1 for that. In scikit-learn also 1 is used as default.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 9, 2017

Member

Fair, compatibility here is probably a good idea. So I agree we should leave it as 1.

Member

rcurtin commented Aug 9, 2017

Fair, compatibility here is probably a good idea. So I agree we should leave it as 1.

@rcurtin rcurtin merged commit 69fd7fb into mlpack:master Aug 10, 2017

3 checks passed

Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@micyril micyril changed the title from Precision, recall and F1 for the cross-validation and hyper-parameter tuning modules to Add precision, recall and F1 for the cross-validation and hyper-parameter tuning modules Aug 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment