Metric sweeping

[SkafteNicki]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

What does this PR do?

Fixes #148
Introduce the higher_is_better static property that determines if a metric is optimal when it is maximized (True), minimized (False) or this is undefined (None).
Still missing adding some metrics were I am not sure what the property should be.
cc: @maximsch2

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #544 (ad1c2cc) into master (ac52dd7) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #544   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         130    130           
  Lines        4618   4632   +14     
=====================================
+ Hits         4400   4411   +11     
- Misses        218    221    +3     

[Borda]

I think it shall be property so it is not possible to change it, the same as is differentiable...
also, do we have another name candidate, this sounds a bit hacky to me...
@PyTorchLightning/core-metrics @justusschock @maximsch2

[SkafteNicki]

Fine with this being a property instead. Only reason I wanted this to be a class attribute was to give the user the possibility of actually getting this value even before the class was initialized.
Name was suggested by @maximsch2

[SkafteNicki]

@lucadiliello what is the correct value for all our retrieval metrics (you are the expert)?
Is a model optimal when they are maximized or minimized or is it not well defined?

[lucadiliello]

@lucadiliello what is the correct value for all our retrieval metrics (you are the expert)?
Is a model optimal when they are maximized or minimized or is it not well defined?

Hi @SkafteNicki , all retrieval metrics should have higher_is_better=True apart from FallOut, which should be minimised.

[Borda]

Only reason I wanted this to be a class attribute was to give the user the possibility of actually getting this value even before the class was initialized.

that is a good point but in the same light why the is_differentiable would be needed first to be initialized?

we can ban certain attributes from being overwritten as for example:

    def _ _setattr_ _(self, name, value):
        if name in ("higher_is_better", "is_diffrenetiable"):
            raise RuntimeError(f"Can't chnage const `{name}`."
        self._ _dict_ _[name] = value

[SkafteNicki]

Only reason I wanted this to be a class attribute was to give the user the possibility of actually getting this value even before the class was initialized.

that is a good point but in the same light why the is_differentiable would be needed first to be initialized?

we can ban certain attributes from being overwritten as for example:

    def _ _setattr_ _(self, name, value):
        if name in ("higher_is_better", "is_diffrenetiable"):
            raise RuntimeError(f"Can't chnage const `{name}`."
        self._ _dict_ _[name] = value

You are completely right. I think we should also change is_differentiable to be this way and it looks like a good solution to me :]

[SkafteNicki]

Anyone got a better name? On top of my head

• 🎉 higher_is_better: options True, False, None
• 👍 maximize (or minimize): options True, False, None
• ❤️ direction or optim_direction: options maximize, minimize, None
• ...

cc: @PyTorchLightning/core-metrics @PyTorchLightning/core-contributors

[SkafteNicki]

@Borda please wait with merge, going to add the property to some more metrics

[tchaton]

LGTM !

[Borda]

@SkafteNicki mind check the failing tests on GPU?