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

Distributed ndcg #3054

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Distributed ndcg #3054

wants to merge 9 commits into from

Conversation

ili0820
Copy link

@ili0820 ili0820 commented Aug 31, 2023

Fixes #2632

This PR is based on #2632 (thanks a lot to kamalojasv181!)

Description:

I ve tried to fix distributed NDCG tests like the examples. I am not really sure whether this is the way you guys wanted, but still, I tried my best and let me know which parts should be fixed (It's my first time contributing, so be kind plz 😯 )

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: metrics Metrics module label Aug 31, 2023
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for finalizing this PR @ili0820 !
I left few comments on how to make CI happy and improve a bit the tests

tests/ignite/metrics/test_ndcg.py Outdated Show resolved Hide resolved
tests/ignite/metrics/test_ndcg.py Outdated Show resolved Hide resolved
tests/ignite/metrics/test_ndcg.py Outdated Show resolved Hide resolved
ignite/metrics/__init__.py Outdated Show resolved Hide resolved
Comment on lines +52 to +54
discounted_gains = torch.tensor(
[_tie_averaged_dcg(y_p, y_t, discount_cumsum, device) for y_p, y_t in zip(y_pred, y_true)], device=device
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

@ili0820 ili0820 Sep 3, 2023

Choose a reason for hiding this comment

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

@vfdev-5 while studying about this, I found out that sklearn is using for loop too. is there any particular reason why this for loop need to be changed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is purely perf reasons. For example, computing s1 below will be faster than s2:

tensor = torch.rand(100)
s1 = tensor.sum()

s2 = 0
for v in tensor:
    s2 += v

tests/ignite/metrics/test_ndcg.py Show resolved Hide resolved
return normalized_gain


class NDCG(Metric):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we need to write a docstring like here:

class Accuracy(_BaseClassification):

Please read this section of contributing guide: https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#writing-documentation, especially about .. versionadded::

@ili0820
Copy link
Author

ili0820 commented Aug 31, 2023

I ve deleted "test_output_cuda", added "n_epoch" and alligned code format for now. I will work on vectorizing and wrting docstring @vfdev-5

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 17, 2023

@ili0820 any blockers that we can help to move forward with this PR ?

@ili0820
Copy link
Author

ili0820 commented Sep 17, 2023

@vfdev-5 sorry for no updates for a long time, I was still struggling with vectorizing for loop. 😢

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 18, 2023

@ili0820 no worries, let's update the docstring and merge this PR. Vectorization part could be done later in a follow-up PR.

@ili0820
Copy link
Author

ili0820 commented Sep 18, 2023

@vfdev-5 wow sounds great. does it matter if i copy and paste docstring from sklearn or other opensource?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 18, 2023

@ili0820 yes, you can inspire from scikit-learn to write ignite's docstring. Please check this contrib part: https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#writing-documentation
You can take this metric https://github.com/pytorch/ignite/blob/master/ignite/metrics/psnr.py as an example

@ili0820
Copy link
Author

ili0820 commented Sep 18, 2023

@vfdev-5 thanks I ll look into that. 😄

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 20, 2023

@ili0820 can you please also add the metric to this list: https://github.com/pytorch/ignite/blob/master/docs/source/metrics.rst

Also please fix formatting issues: https://github.com/pytorch/ignite/actions/runs/6251715047/job/16981806158?pr=3054#step:11:104

@ili0820 ili0820 closed this Sep 21, 2023
@ili0820 ili0820 deleted the distributed-ndcg branch September 21, 2023 00:42
@ili0820
Copy link
Author

ili0820 commented Sep 21, 2023

@vfdev-5 I accidently deleted the branch................................ 🤢
will try to restore....

@ili0820
Copy link
Author

ili0820 commented Sep 21, 2023

restored. sorry for inconvenience

@ili0820 ili0820 reopened this Sep 21, 2023
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 3, 2023

@ili0820 can you update the PR according to the review, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: metrics Metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants