Add mrr calculation #886

zkid18 · 2020-07-15T12:57:49Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contribution guide?
Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
Did you make sure to update the docs? We use Google format for all the methods and classes.
Did you check the docs with make check-docs?
Did you write any new necessary tests?
Did you add your new functionality to the docs?
Did you update the CHANGELOG?
You can use 'Login as guest' to see Teamcity build logs.

Description

Related Issue

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

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.

pep8speaks · 2020-07-15T12:57:52Z

Hello @zkid18! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-11 10:49:27 UTC

mergify · 2020-07-15T14:48:19Z

This pull request is now in conflicts. @zkid18, could you fix it? 🙏

Scitator · 2020-07-15T16:46:58Z

catalyst/utils/metrics/mrr.py

+import torch
+
+
+def mrr(outputs: torch.Tensor, targets: torch.Tensor):


could you please add docs here
https://github.com/catalyst-team/catalyst/blob/master/docs/api/utils.rst
?

btw, what do you think about MRRCallback? like https://github.com/catalyst-team/catalyst/blob/master/catalyst/dl/callbacks/metrics/iou.py#L12 for example

Thanks, I'll have a look at callbacks.

zkid18 · 2020-07-16T06:49:18Z

I still consider the proper design for MRR@K. Probably _metric@k is quite common task, so rather than implement this functionality inside every metrics extend _metric with, say, topKMixin

Scitator · 2020-07-19T08:26:23Z

catalyst/utils/metrics/mrr.py

+        mrr (float): the mrr score
+    """
+    outputs = outputs.clone()
+    targets = targets.clone()


why do you need clone?

Tried to follow the 'shared clones', which is a common pattern in torch. But it seems it not so necessary here.

Scitator

btw, looks like we need to fix codestyle

Scitator · 2020-07-26T08:59:17Z

catalyst/utils/metrics/mrr.py

+import torch
+
+
+def mrr(outputs: torch.Tensor, 


could you please add this metric to docs?
https://github.com/catalyst-team/catalyst/blob/master/docs/api/utils.rst#metrics

Scitator · 2020-07-26T09:00:44Z

catalyst/dl/callbacks/metrics/mrr.py

+from catalyst.utils import metrics
+
+
+class MRRCallback(MetricCallback):


could you please add this callback to the docs?
https://github.com/catalyst-team/catalyst/blob/master/docs/api/dl.rst#metrics

As for tests, I think better return to the question when we implement at least one Learning to Rank models.

Pull request has been modified.

mergify · 2020-07-28T17:52:22Z

This pull request is now in conflicts. @zkid18, could you fix it? 🙏

Scitator · 2020-07-30T06:12:53Z

catalyst/dl/callbacks/metrics/mrr.py

+            output_key (str): output key to use for auc calculation;
+                specifies our ``y_pred``
+            prefix (str): name to display for mrr when printing
+            activation (str): An torch.nn activation applied to the outputs.


why do we need activation? mrr metric doesn't have activation support

Scitator · 2020-07-30T06:14:40Z

@zkid18 could you please add a sanity check test for MRRCallback? like "init and compute on one sample"

bagxi · 2020-09-10T06:38:17Z

catalyst/utils/metrics/__init__.py

 from catalyst.utils.metrics.auc import auc
 from catalyst.utils.metrics.cmc_score import cmc_score_count, cmc_score
 from catalyst.utils.metrics.dice import dice, calculate_dice
 from catalyst.utils.metrics.f1_score import f1_score
+from catalyst.utils.metrics.mrr import mrr


could you please follow the alphabetical order of imports?

bagxi · 2020-09-10T06:39:42Z

catalyst/dl/callbacks/metrics/tests/test_mrr.py

+    num_epochs=2,
+    verbose=True,
+    callbacks=[MRRCallback, SchedulerCallback(reduced_metric="loss")]
+)


new line at the end of py file is missed

bagxi · 2020-09-10T06:40:48Z

catalyst/utils/metrics/mrr.py

+import torch
+
+
+def mrr(outputs: torch.Tensor, targets: torch.Tensor, k=100) -> torch.Tensor:


could you please define the types for all args?

bagxi · 2020-09-10T06:43:56Z

catalyst/utils/metrics/mrr.py

+            The mrr score for each user.
+
+    """
+    k = min(outputs.size()[1], k)


Suggested change

k = min(outputs.size()[1], k)

k = min(outputs.size(1), k)

bagxi · 2020-09-10T06:50:10Z

catalyst/utils/metrics/mrr.py

+def mrr(outputs: torch.Tensor, targets: torch.Tensor, k=100) -> torch.Tensor:
+
+    """
+    Calculate the MRR score given model ouptputs and targets


I'd appreciate it if you could extend documentation with the explanation of the metric or add link users will be able to read more about this score.

bagxi · 2020-09-10T06:52:58Z

catalyst/utils/metrics/mrr.py

+
+    """
+    k = min(outputs.size()[1], k)
+    _, indices_for_sort = outputs.sort(descending=True, dim=-1)


could we use torch.topk here?

I guess the comment is more relevant to the next part
true_sorted_by_pred_shrink = true_sorted_by_preds[:, :k]

Anyway, I might missing the advantage of torch.topk over the proposed approach.
We need to sort predictions by the corresponding indexes of the outputs. Is there is a way in pytorch to sort in that fashion?

Pull request has been modified.

Scitator · 2020-09-29T15:43:22Z

CHANGELOG.md

@@ -39,7 +39,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 ## [20.08] - 2020-08-09

 ### Added
- Full metric learning pipeline including training and validation stages ([#886](https://github.com/catalyst-team/catalyst/pull/876))
+- MRR metrics calculation ([#886](https://github.com/catalyst-team/catalyst/pull/886))


looks like we need to move it up :)

Scitator · 2020-10-04T07:30:44Z

@zkid18 could you please update your branch and resolver the above questions? thank you!

Updated the master branch

Pull request has been modified.

zkid18 · 2020-10-08T04:41:16Z

@Scitator have fixed the typos, and moved mrr metric under metrics folder.
Could you check if I understood this idea correctly?

zkid18 · 2020-10-08T06:19:16Z

@Scitator
Not sure if this problem related to codestyle itself or rather actions pipeline.
https://github.com/catalyst-team/catalyst/runs/1224097493?check_suite_focus=true

Scitator · 2020-10-09T07:03:31Z

docs/api/utils.rst

+
+MRR
+~~~~~~~~~~~~~~~~~~~~~~
+.. automodule:: catalyst.utils.metrics.mrr


looks like here is an error with docs :)

Pull request has been modified.

Daniel Chepenko added 4 commits July 14, 2020 18:02

mrr implementation

8163316

add mrr

17a4b03

edit codestyle

0f1151a

Add changelog

93c637e

zkid18 requested review from bagxi and Scitator as code owners July 15, 2020 12:57

updated changelog

f6d7496

Scitator reviewed Jul 15, 2020

View reviewed changes

add docstring to mrr

4c46f25

Scitator reviewed Jul 19, 2020

View reviewed changes

Daniel Chepenko added 5 commits July 25, 2020 21:48

fixed commit and pep8

21a36b0

removed clones

ae9089c

Add batch tests

6b765aa

edit changelog

a69273f

Add callbacks

c425ccf

Scitator previously requested changes Jul 26, 2020

View reviewed changes

make codestyle

668196f

Daniel Chepenko added 4 commits July 27, 2020 21:12

small issues

b626eeb

add newline at the end of the file

7f33538

small issues

06e6761

add movielens

04caa31

minor improvements

d9d7764

Scitator previously requested changes Jul 30, 2020

View reviewed changes

minor improvements

8bf6700

Daniel Chepenko added 7 commits August 5, 2020 20:32

Merge remote-tracking branch 'upstream/master'

e870498

updated changelog

ba4c180

add at k support

80920f0

Merge remote-tracking branch 'upstream/master' into recsys-mrr

cf8828f

add mrr computations

84b7b52

commit before merge

28c5148

Merge branch 'master' into recsys-mrr

8911528

zkid18 requested a review from ditwoo as a code owner September 6, 2020 07:21

Daniel Chepenko added 3 commits September 6, 2020 16:22

deleted dataset from another branch

3acb341

WIP merr calcback tests

5a16261

minor changes

e192837

bagxi previously requested changes Sep 10, 2020

View reviewed changes

Daniel Chepenko added 3 commits September 12, 2020 18:44

alphabetical order of the imports

2230a5b

add new line at the end of py file

f632cc1

fixed small issues

470cc99

changed the codestyle

8127eb8

Scitator previously approved these changes Sep 29, 2020

View reviewed changes

Daniel Chepenko added 4 commits October 8, 2020 12:49

Merge remote-tracking branch 'upstream/master'

034b832

Merge branch 'master' into recsys-mrr

6dad3f4

Updated the master branch

moved files to metrics

b2fa81a

fixed typos

5f51a38

Scitator previously requested changes Oct 9, 2020

View reviewed changes

fixed docs

4947d4b

Scitator merged commit dbc94a7 into catalyst-team:master Oct 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mrr calculation #886

Add mrr calculation #886

zkid18 commented Jul 15, 2020

pep8speaks commented Jul 15, 2020 •

edited

mergify bot commented Jul 15, 2020

Scitator Jul 15, 2020

Scitator Jul 15, 2020

zkid18 Jul 16, 2020 •

edited

zkid18 commented Jul 16, 2020

Scitator Jul 19, 2020

zkid18 Jul 23, 2020

Scitator left a comment

Scitator Jul 26, 2020

Scitator Jul 26, 2020

zkid18 Sep 29, 2020

mergify bot commented Jul 28, 2020

Scitator Jul 30, 2020

Scitator commented Jul 30, 2020 •

edited

bagxi Sep 10, 2020

bagxi Sep 10, 2020

bagxi Sep 10, 2020

bagxi Sep 10, 2020

bagxi Sep 10, 2020

bagxi Sep 10, 2020

zkid18 Sep 22, 2020

Scitator Sep 29, 2020

Scitator commented Oct 4, 2020

zkid18 commented Oct 8, 2020

zkid18 commented Oct 8, 2020

Scitator Oct 9, 2020

		import torch


		def mrr(outputs: torch.Tensor, targets: torch.Tensor):

		from catalyst.utils import metrics


		class MRRCallback(MetricCallback):

Add mrr calculation #886

Add mrr calculation #886

Conversation

zkid18 commented Jul 15, 2020

Before submitting

Description

Related Issue

Type of Change

PR review

pep8speaks commented Jul 15, 2020 • edited

Comment last updated at 2020-10-11 10:49:27 UTC

mergify bot commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkid18 Jul 16, 2020 • edited

Choose a reason for hiding this comment

zkid18 commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scitator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jul 28, 2020

Choose a reason for hiding this comment

Scitator commented Jul 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scitator commented Oct 4, 2020

zkid18 commented Oct 8, 2020

zkid18 commented Oct 8, 2020

Choose a reason for hiding this comment

pep8speaks commented Jul 15, 2020 •

edited

zkid18 Jul 16, 2020 •

edited

Scitator commented Jul 30, 2020 •

edited