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

Return also classes for MAP metric #1419

Merged
merged 9 commits into from
Jan 17, 2023
Merged

Return also classes for MAP metric #1419

merged 9 commits into from
Jan 17, 2023

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Dec 30, 2022

What does this PR do?

Fixes #1417
Returns also classes for MAP metric.

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?

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 🙃

@SkafteNicki SkafteNicki added the enhancement New feature or request label Dec 30, 2022
@SkafteNicki SkafteNicki added this to the v0.12 milestone Dec 30, 2022
@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #1419 (7cf0962) into master (4f3cab9) will decrease coverage by 54%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1419     +/-   ##
========================================
- Coverage      90%     36%    -54%     
========================================
  Files         211     211             
  Lines       10849   10850      +1     
========================================
- Hits         9719    3870   -5849     
- Misses       1130    6980   +5850     

@mergify mergify bot added the ready label Dec 30, 2022
@Borda Borda enabled auto-merge (squash) January 6, 2023 02:09
@Borda Borda requested a review from a team January 6, 2023 02:14
@Borda Borda merged commit ac64e63 into master Jan 17, 2023
@Borda Borda deleted the feature/n_class_map branch January 17, 2023 03:20
@adamjstewart
Copy link
Contributor

Unfortunately this makes it much more difficult to use this metric in Lightning.

The following code:

class Foo(LightningModule):
    def __init__(self):
        self.metric = MeanAveragePrecision()

    def on_validation_epoch_end(self):
        self.log_dict(self.metric.compute())

used to work in 0.11.4. However, this PR adds a new classes key-pair to the metrics dict containing a list of all observed classes. log_dict now complains about this entry:

ValueError: `self.log(classes, tensor([0, 1], dtype=torch.int32))` was called, but the tensor must have a single element. You can try doing `self.log(classes, tensor([0, 1], dtype=torch.int32).mean())`

Here, tensor([0, 1]) is the classes value. Current workaround is to delete this key from the dict before logging, but it would be good to do this automatically.

@Borda Borda modified the milestones: v1.0.0, v1.0.x Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return class_names as part of MeanAveragePrecision.compute()
4 participants