[bugfix] MetricCollection should return metrics with prefix on items(), keys() #209

tchaton · 2021-04-28T16:18:53Z

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?

This PR makes sure items() / keys() properly returns the right keys with prefix / postfix.

Fixes #Lightning-AI/pytorch-lightning#7235.

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 🙃

pep8speaks · 2021-04-28T16:18:56Z

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-05-03 20:05:29 UTC

for more information, see https://pre-commit.ci

codecov · 2021-04-28T16:20:42Z

Codecov Report

Merging #209 (a1596a6) into master (7a9e50a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   96.73%   96.76%   +0.02%     
==========================================
  Files         184      184              
  Lines        5888     5930      +42     
==========================================
+ Hits         5696     5738      +42     
  Misses        192      192

Flag	Coverage Δ
Linux	`79.66% <55.55%> (-0.17%)`	⬇️
Windows	`79.66% <55.55%> (-0.17%)`	⬇️
cpu	`96.76% <100.00%> (+0.02%)`	⬆️
gpu	`96.76% <ø> (+0.02%)`	⬆️
macOS	`96.76% <100.00%> (+0.02%)`	⬆️
pytest	`96.76% <100.00%> (+0.02%)`	⬆️
python3.6	`95.69% <100.00%> (+0.03%)`	⬆️
python3.8	`96.72% <100.00%> (+0.02%)`	⬆️
python3.9	`?`
torch1.3.1	`95.69% <100.00%> (+0.03%)`	⬆️
torch1.4.0	`95.81% <100.00%> (+0.02%)`	⬆️
torch1.8.1	`96.62% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
torchmetrics/collections.py	`98.82% <100.00%> (+0.38%)`	⬆️
__w/2/s/torchmetrics/collections.py	`98.82% <0.00%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a9e50a...a1596a6. Read the comment docs.

…ing/metrics into metrics_collection

itsikad · 2021-04-28T16:38:46Z

Thanks @tchaton

I'm probably missing something here, but why not changing the clone method such that it adds prefix/postfix to the returned dictionary keys?

SkafteNicki

LGTM

* update pre-commit * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Apply suggestions from code review Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

* fix setup imports * pkg

…ing/metrics into metrics_collection

pls check last version

torchmetrics/collections.py

maximsch2 · 2021-04-29T16:45:33Z

@tchaton , I agree that we should fix the checkpointing behavior, but my point is that it might be cleaner to just change the clone method so that the new collection actually has the keys with prefixes inside, that way we don't have to have hacks that pretend otherwise at run time.

tchaton · 2021-04-30T09:07:25Z

@tchaton , I agree that we should fix the checkpointing behavior, but my point is that it might be cleaner to just change the clone method so that the new collection actually has the keys with prefixes inside, that way we don't have to have hacks that pretend otherwise at run time.

Hey @SkafteNicki. What are your thoughts ? Happy to let someone else change the MetricCollection to save the metrics with prefix and postfix directly.

Best,
T.C

SkafteNicki · 2021-04-30T12:53:51Z

@tchaton I am in favour of this change so the keys are consistent between .items(), .keys() and the dics returned by forward and .compute()

maximsch2 · 2021-05-03T17:32:44Z

Actually I now think this is fine - we can just flip the implementation later and just do the in-place modification in case of keep_base=True.

lgtm

Borda · 2021-05-04T06:33:24Z

Actually I now think this is fine - we can just flip the implementation later and just do the in-place modification in case of keep_base=True.

so pls make an issue for it for this milestone so we do not forget it... (and link it in this PR)

…), keys() (#209) * update * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update print * resolve flake8 * update * CI: update pre-commit (#207) * update pre-commit * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Apply suggestions from code review Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> * fix setup imports (#208) * fix setup imports * pkg * chlog * format * update * update * check type * update on comments * update on comments Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Nicki Skafte <skaftenicki@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: jirka <jirka.borovec@seznam.cz> (cherry picked from commit 67ce961)

update

b5d69b7

tchaton added this to the v0.4 milestone Apr 28, 2021

tchaton self-assigned this Apr 28, 2021

tchaton requested review from ananyahjha93, Borda, justusschock and SkafteNicki as code owners April 28, 2021 16:18

[pre-commit.ci] auto fixes from pre-commit.com hooks

137da8e

for more information, see https://pre-commit.ci

tchaton requested a review from edenlightning as a code owner April 28, 2021 16:19

tchaton added 4 commits April 28, 2021 17:28

update print

c7a4008

resolve flake8

5ae8d8e

Merge branch 'metrics_collection' of https://github.com/PyTorchLightn…

8a5156f

…ing/metrics into metrics_collection

update

6af03f3

Merge branch 'master' into metrics_collection

953e8ab

SkafteNicki approved these changes Apr 28, 2021

View reviewed changes

Merge branch 'master' into metrics_collection

5deb58f

Borda added the bug / fix Something isn't working label Apr 28, 2021

Borda modified the milestones: v0.4, v0.3 Apr 28, 2021

Borda and others added 4 commits April 28, 2021 19:59

fix setup imports (#208)

77614f6

* fix setup imports * pkg

chlog

9ed3315

Merge branch 'metrics_collection' of https://github.com/PyTorchLightn…

fcadfc0

…ing/metrics into metrics_collection

Borda previously approved these changes Apr 28, 2021

View reviewed changes

format

10e1c84

Borda enabled auto-merge (squash) April 28, 2021 18:04

update

76c5f49

check type

dc9d12a

Borda requested a review from carmocca April 29, 2021 08:14

carmocca reviewed Apr 29, 2021

View reviewed changes

torchmetrics/collections.py Outdated Show resolved Hide resolved

tchaton added 2 commits April 29, 2021 13:15

update on comments

ab19250

update on comments

0076b69

carmocca approved these changes Apr 29, 2021

View reviewed changes

SkafteNicki enabled auto-merge (squash) April 29, 2021 12:26

SkafteNicki added the ready label Apr 29, 2021

Merge branch 'master' into metrics_collection

685061b

tchaton and others added 4 commits April 30, 2021 14:56

Merge branch 'master' into metrics_collection

0a88348

Merge branch 'master' into metrics_collection

ebe9d98

Merge branch 'master' into metrics_collection

281d22d

Merge branch 'master' into metrics_collection

79fcf67

awaelchli approved these changes May 3, 2021

View reviewed changes

Merge branch 'master' into metrics_collection

12e90fc

Borda self-requested a review May 3, 2021 17:16

mergify bot added the has conflicts label May 3, 2021

Merge branch 'master' into metrics_collection

b349ae6

mergify bot removed the has conflicts label May 3, 2021

mergify bot added 2 commits May 3, 2021 18:15

Merge branch 'master' into metrics_collection

2f2dfae

Merge branch 'master' into metrics_collection

a1596a6

SkafteNicki merged commit 67ce961 into master May 4, 2021

SkafteNicki deleted the metrics_collection branch May 4, 2021 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] MetricCollection should return metrics with prefix on items(), keys() #209

[bugfix] MetricCollection should return metrics with prefix on items(), keys() #209

tchaton commented Apr 28, 2021 •

edited by Borda

Loading

pep8speaks commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

itsikad commented Apr 28, 2021 •

edited

Loading

SkafteNicki left a comment

maximsch2 commented Apr 29, 2021

tchaton commented Apr 30, 2021

SkafteNicki commented Apr 30, 2021

maximsch2 commented May 3, 2021

Borda commented May 4, 2021

[bugfix] MetricCollection should return metrics with prefix on items(), keys() #209

[bugfix] MetricCollection should return metrics with prefix on items(), keys() #209

Conversation

tchaton commented Apr 28, 2021 • edited by Borda Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Apr 28, 2021 • edited Loading

Comment last updated at 2021-05-03 20:05:29 UTC

codecov bot commented Apr 28, 2021 • edited Loading

Codecov Report

itsikad commented Apr 28, 2021 • edited Loading

SkafteNicki left a comment

Choose a reason for hiding this comment

maximsch2 commented Apr 29, 2021

tchaton commented Apr 30, 2021

SkafteNicki commented Apr 30, 2021

maximsch2 commented May 3, 2021

Borda commented May 4, 2021

tchaton commented Apr 28, 2021 •

edited by Borda

Loading

pep8speaks commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

itsikad commented Apr 28, 2021 •

edited

Loading