Conversation
|
@steaphenai In |
6e158d3 to
e910463
Compare
a6edab5 to
cbbc275
Compare
4f66145 to
65d7557
Compare
@aaishwarymishra Thanks!! |
Added ranx = pytest.importorskip(...) to make ranx optional Wrapped ranx validation checks with if ranx is not None Added pytest.skip() for tests that specifically require ranx Tests now pass without ranx installed while still validating against it when available. The NDCG metric implementation itself has no ranx dependency.
Use try/except to handle missing ranx dependency instead of pytest.importorskip
|
@steaphenai I was checking your implementation compared to previous ones where contributors were blocked with vectorized computation of 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
)when ignore_ties is False. I can't find the same thing in your implementation, can you explain your implementation, what it based on? |
@vfdev-5 My implementation is based on the standard NDCG formula and validated against the ranx library. |
|
@vfdev-5 Just to clarify on the vectorization part, my implementation is fully vectorized using PyTorch operations (no Python loops). Instead of looping through each example like: [_tie_averaged_dcg(y_p, y_t, ...) for y_p, y_t in zip(y_pred, y_true)]I use batched PyTorch operations: ranked_indices = torch.argsort(y_pred, dim=-1, descending=True, stable=True)[:, :max_k]
ranked_relevance = torch.gather(y_for_dcg, dim=-1, index=ranked_indices)This processes all examples in the batch simultaneously, which should be more efficient than the loop-based approach. |
|
To help with reviewing can you please run catalyst and ranx on some non-trivial examples involving ties etc and compare results. |
Removed try/except import and conditional checks for ranx. ranx is now a required dev dependency in requirements-dev.txt, so tests can use it directly without fallback logic.
Co-authored-by: vfdev <vfdev.5@gmail.com>
Add detailed explanation of relevance types for NDCG.
|
@vfdev-5 I added |
Added a tutorial link for using the NDCG metric.
Added usage tutorial reference for NDCG metric.
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
|
@steaphenai please check the failing CI job: https://github.com/pytorch/ignite/actions/runs/23947698404/job/70144090725?pr=3608 |
@vfdev-5 I verified with pre-commit run --all-files. All the tests were passed locally. |
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks for updates @steaphenai
We almost there, just few updates
Updated NDCG test functions to use ranx for verification and adjusted imports.
@vfdev-5 Done! I've set the imports to the top of the file. |
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR @steaphenai , LGTM
|
@steaphenai still failing CI code style check, please fix it |
|
@vfdev-5 I moved the Catalyst import to the top-level as requested, but TPU CI was failing during test collection because importing catalyst triggers a |
|
Catalyst is unmaintained now, so I was actually thinking if we can remove it from the tests and keep only ranx (which is also look a bit left aside). |
|
@vfdev-5 Makes sense. I agree we should remove Catalyst-based parity tests and keep ranx-based validation. I’ll drop Catalyst imports/helpers/tests and keep the ranx checks as the reference path |
Head branch was pushed to by a user without write access
Fixes #3581
Refs #3568 #3569 #3610
Description: Implements NDCG (Normalized Discounted Cumulative Gain) metric for recommendation systems and ranking evaluation.
Implementation Details
HitRateandMRRmetricsranxlibrary for verification (addressing [Feature]: Add Mean Reciprocal Rank (MRR) metric to rec_sys #3569 feedback)relevance_thresholddefaults to 1.0 for binary labelsstable=Truefor reproducible tie-breakingMathematical Background
Formula:
NDCG@K = Σ(2^rel_i - 1) / log2(i + 1)Testing
Check list: