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.

Sign up for GitHub

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

Jump to bottom

Fix hitrate #1159

Merged
merged 39 commits into from Apr 9, 2021
Merged

Fix hitrate #1159

merged 39 commits into from Apr 9, 2021

Conversation

zkid18
Copy link
Contributor

@zkid18 zkid18 commented Apr 4, 2021

Before submitting (checklist)

  • 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 check that your code passes the unit tests pytest . ?
  • Did you add your new functionality to the docs?
  • Did you update the CHANGELOG?
  • Did you run colab minimal CI/CD with latest and minimal requirements?

Description

Related Issue

#1155

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.

PS

  • I know, that I could join slack for pull request discussion.

Daniel Chepenko and others added 30 commits July 28, 2020 19:44
add movielens
04caa31
Merge remote-tracking branch 'upstream/master'
e870498
commit before merge
28c5148
Merge remote-tracking branch 'upstream/master'
034b832
@denyhoof
First hitrate metric version
699e97d
Merge remote-tracking branch 'upstream/master'
ef3ce1e
Merge remote-tracking branch 'upstream/master'
9dae326
@zkid18
Merge remote-tracking branch 'upstream/master'
3c5e53a
@zkid18
metrge with upstream
38abe3a
@zkid18
fix topk_map
2e7e891
@zkid18
fix error in map
1c89f51
@zkid18
tests for map
56a4186
@zkid18
top_k ndcg
3c7079a
@zkid18
edit changelog
372e877
@zkid18
edit the tabs
c5f4435
@zkid18
check codestyle
2e1d957
@zkid18
check the intent
2402682
@zkid18
check the intent
534d482
@zkid18
remove trailing whitespace
f3e927b
@zkid18
fixed hitrate
c35a6a3
@zkid18
fixed the docs
e81e31f
@zkid18
hitrate
2a60b31
@zkid18
Merge remote-tracking branch 'upstream/master'
9b6c04a
@Atharva-Phatak @zkid18
Additive margin softmax (catalyst-team#1131)
560188e 
* Added Additive-margin softmax

* update changelog.md
@Scitator @zkid18
Readme update2 (catalyst-team#1142)
767b031 
* readme

* batch overfit fix for reproducibility

* batch overfit fix for reproducibility

* readme

* readme update

* extra Config/Hydra API fixes

* codestyle
@Atharva-Phatak @Scitator @zkid18
updated dl_cpu(workflows)- For passing CI-Tests (catalyst-team#1135)
b1ad3e4 
* updated dl_cpu

* Updated-requirements-ml.txt

* corrected the mistake

* Update dl_cpu.yml

* Update requirements-ml.txt

* Kept dl_cpu 20.04-3.6 minimal as requested

* Corrected the mistakes

* Add requirements parameter

* Update dl_cpu.yml

* Update dl_cpu.yml

Co-authored-by: Sergey Kolesnikov <scitator@gmail.com>
@bagxi @zkid18
fix: _key_value for schedulers in case of multiple optimizers fixed (…
a5334fe 
…catalyst-team#1146)
@Scitator @elephantmipt @zkid18
Github CI fixes (catalyst-team#1143)
c929ab5 
* verbose

* Utils callbacks (catalyst-team#1127)

* quantization.py

* onnx and quantization

* tracing.py

* docs

* fix

* fix

* quantization test

* fix test

* fix comments

* fix comments

* Update catalyst/callbacks/__init__.py

Co-authored-by: Sergey Kolesnikov <scitator@gmail.com>

* tests

* tests

* tests

* tests

* tests

* pycharm tricks

* extra tests

* Utils callbacks (catalyst-team#1145)

* quantization.py

* onnx and quantization

* tracing.py

* docs

* fix

* fix

* quantization test

* fix test

* fix comments

* fix comments

* quantization example

* examples

* CHANGELOG.md

* fix

* fix

* Apply suggestions from code review

Co-authored-by: Sergey Kolesnikov <scitator@gmail.com>

* extra tests 2

* extra tests 2

* extra tests 2

* extra tests 2

* extra tests 2

* extra tests 2

* extra tests 2

* extra tests 2

Co-authored-by: Nikita Balagansky <37884009+elephantmipt@users.noreply.github.com>
@Scitator @ditwoo @zkid18
Engine docs (catalyst-team#1141)
b40d166 
* typings & few docs

* typing fix; disabled `dist.barrier()` in optimizer step for ddp

* docs

* docs: fixed long lines with docs

* docs fixes

* optimizer args

* removed empty line

Co-authored-by: Dmytro Doroshenko <dimdoroshenko@gmail.com>
@Scitator @zkid18
v21.03.1
bd2c46b
Scitator and others added 7 commits April 4, 2021 12:46
@Scitator @zkid18
v21.03: minimal version fix (catalyst-team#1147)
c0bc738 
* minimal version fix

* docs
@bagxi @zkid18
fix: nested dicts in loaders_params/samplers_params overriding (catal…
5a15977 
…yst-team#1150)
@elephantmipt @zkid18
docs (catalyst-team#1152)
fcbcfd1
@Scitator @zkid18
Github CI fix (catalyst-team#1148)
cd96186 
* fix

* fix
@ssktotoro @zkid18
Niftireader (catalyst-team#1151)
074534b 
* changes for NiftiReader

* adding import statement that was needed and fixing codestyle

* updated CHANGELOG

* removing unnecesary documentation

* fixing codestyle

* updated docs and requirements

* adding a space

* not importing nibabel for docs

* fixing typo

* adding step for contrib pip caching

* fixed codestyle and added a test for reader that uncovered a mistake :)

* hopefully fixing imports

* adding sergey suggested fix

* fixing deploy_push.yml

* updating workflows

Co-authored-by: Kevin Wang <kevin.wang@affectiva.com>
Co-authored-by: Kevin Wang <kcwang26@gsu.edu>
@zkid18
Merge branch 'master' into fix_topk_metrics
e906e1e
@zkid18
add changelog
c8ba69b
@vinnamkim
Copy link

@Scitator LGTM

Scitator
Scitator previously requested changes Apr 5, 2021
View reviewed changes
catalyst/metrics/functional/_hitrate.py Outdated
@@ -39,7 +40,10 @@ def hitrate(outputs: torch.Tensor, targets: torch.Tensor, topk: List[int]) -> Li
targets_sort_by_outputs = process_recsys_components(outputs, targets)
for k in topk:
k = min(outputs.size(1), k)
hits_score = torch.sum(targets_sort_by_outputs[:, :k], dim=1) / k
if targets.sum(dim=1).data[0] != 0:
Copy link
Member

Choose a reason for hiding this comment

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

not sure if this is correct

@mergify mergify bot dismissed Scitator’s stale review April 9, 2021 11:08

Pull request has been modified.

@Scitator Scitator merged commit 9e381c8 into catalyst-team:master Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Scitator Scitator Scitator left review comments

@bagxi bagxi Awaiting requested review from bagxi bagxi is a code owner

@ditwoo ditwoo Awaiting requested review from ditwoo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@zkid18 @vinnamkim @Scitator @denyhoof @Atharva-Phatak @bagxi @elephantmipt @ssktotoro