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

Fix topk metrics #1330

Merged
merged 13 commits into from
Oct 20, 2021
Merged

Fix topk metrics #1330

merged 13 commits into from
Oct 20, 2021

Conversation

sergunya17
Copy link
Contributor

@sergunya17 sergunya17 commented Oct 15, 2021

Pull Request FAQ

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.

Checklist

  • Have you updated tests for the new functionality?
  • Have you added your new classes/functions to the docs?
  • Have you updated the CHANGELOG?
  • Have you run colab minimal CI/CD with latest and minimal requirements?
  • Have you checked XLA integration with single and multiple processes?

@pep8speaks
Copy link

pep8speaks commented Oct 15, 2021

Hello @sergunya17! 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 2021-10-19 19:49:32 UTC

@Scitator Scitator marked this pull request as ready for review October 17, 2021 08:03
Copy link
Member

@Scitator Scitator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add docs on metric logic under self.batch/epoch metrics? For example, when you specify AccuracyCallback, it would add accuracy01 to the metrics storages

catalyst/metrics/_topk_metric.py Show resolved Hide resolved
@mergify mergify bot dismissed Scitator’s stale review October 17, 2021 21:12

Pull request has been modified.

@sergunya17 sergunya17 changed the title Draft: Fix topk metrics Fix topk metrics Oct 17, 2021
@Scitator
Copy link
Member

🤔 we also need to add a new metric to catalyst/metrics/__init__.py

@Scitator Scitator merged commit b41bac0 into catalyst-team:master Oct 20, 2021
@sergunya17 sergunya17 deleted the fix_topk_metrics branch October 24, 2021 13:39
daavoo added a commit to iterative/dvclive that referenced this pull request Nov 3, 2021
Recent catalyst versions changed the default formatting of the metrics, based on topk argument: catalyst-team/catalyst#1330

Close #184
daavoo added a commit to iterative/dvclive that referenced this pull request Nov 3, 2021
Recent catalyst versions changed the default formatting of the metrics, based on topk argument: catalyst-team/catalyst#1330

Close #184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants