Skip to content

fix(ndcg): correct the DCG discount off-by-one (rank 2 no longer scores as rank 1)#229

Merged
himanshu231204 merged 3 commits into
OpenAgentHQ:mainfrom
Nitjsefnie-OSC:fix/ndcg-discount-offbyone
Jul 23, 2026
Merged

fix(ndcg): correct the DCG discount off-by-one (rank 2 no longer scores as rank 1)#229
himanshu231204 merged 3 commits into
OpenAgentHQ:mainfrom
Nitjsefnie-OSC:fix/ndcg-discount-offbyone

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

Fixes #223

_dcg's discount was log2(i + 1) with 0-indexed i and an i == 0 special case, giving rank 2 the same discount as rank 1. Now the standard log2(rank + 1) — i.e. rel / _log2(i + 2), no special case needed since log2(2) = 1.

Hand-derived expectations locked by new tests (single relevant doc of 3, k=3; IDCG = 1.0 so NDCG = the doc's discount): rank 1 → 1.0, rank 2 → 1/log2(3) = 0.6309…, rank 3 → 1/log2(4) = 0.5, strictly decreasing; plus a perfect-ranking test proving IDCG normalization still yields exactly 1.0. Both value tests fail on current main (rank 2 returns 1.0; the monotonicity test hits assert 1.0 > 1.0) and pass with the fix.

One honest note: none of the four existing NDCG tests encoded the shifted values (they assert 1.0/0.0/open ranges that hold under both formulas — which is exactly how the bug survived), so no existing expectations changed; the full suite is green untouched: 962 passed, 4 skipped. The few ruff findings in ndcg.py/test_retrieval.py are pre-existing on main (verified) — new code adds none.

Generated by Claude Fable 5 (brief, review), Claude Opus 4.8 (implementation)

Nitjsefnie and others added 2 commits July 23, 2026 21:05
`_dcg` discounted by `log2(i + 1)` over a 0-indexed `i` with an `i == 0`
special case, so rank 2 (`i == 1`) received discount `log2(2) = 1.0` —
identical to rank 1, i.e. no position penalty. Every position was one slot
too generous and NDCG failed to distinguish ranks 1 and 2.

Discount by the standard `log2(rank + 1)` with 1-indexed rank, i.e.
`log2(i + 2)` here. This yields `log2(2) = 1.0` at rank 1 (no special case
needed) and a strictly increasing discount thereafter. IDCG normalization
uses the same `_dcg`, so a perfectly ranked result still scores exactly 1.0.

Regression: a single relevant doc among 3 (k=3) now scores 1.0 / 0.6309 /
0.5 by rank (strictly decreasing) instead of 1.0 / 1.0 / 0.6309.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@himanshu231204

Copy link
Copy Markdown
Member

/oc review pr

@opencode-agent

Copy link
Copy Markdown
Contributor

Review: Approved.

Bug

_dcg used _log2(i + 1) with a i == 0 special case, making rank 2's discount log2(2) = 1.0 — identical to rank 1. The fix replaces both the special case and i + 1 with i + 2, giving the standard log2(rank + 1) discount with no special-casing.

Code quality

  • Correctness: log2(i + 2) correctly maps to log2(1 + rank) (rank 1 → log2(2) = 1, rank 2 → log2(3) ≈ 1.585, rank 3 → log2(4) = 2). The _log2 guard for x ≤ 0 is still effectively dead code (ranks are never ≤ 0), but removing it would be scope creep — fine to leave.
  • Change surface: +5/-4 lines in _dcg. The _ndcg function uses normalized IDCG from the same _dcg, so the fix automatically applies to both — no inconsistency risk. The docstring and comment were updated; the comment is explanatory without being verbose. Good.
  • Tests: Three new tests (test_dcg_discount_single_relevant_by_rank, test_dcg_ndcg_strictly_decreases_with_rank, test_idcg_normalization_perfect_ranking_is_one) encode hand-derived expectations with pytest.approx. The first two fail on the old code (the PR description is accurate). The third confirms IDCG normalization still yields exactly 1.0, which is the critical "don't break perfect rankings" guard.
  • Existing tests untouched: The four preexisting NDCG tests use == 1.0, 0.0 < x < 1.0, == 0.0, and < 1.0 — all invariant under this change. No risk of regression.

Nit (optional)

Line 29: sum(rel / _log2(i + 2) ...) evaluates _log2(i + 2) repeatedly. Could memoize _log2 for tiny perf gain, but this is an eval-time metric over small-k lists; not worth the complexity.

Ship it.

New%20session%20-%202026-07-23T20%3A46%3A30.112Z
opencode session  |  github run

@himanshu231204

Copy link
Copy Markdown
Member

@Nitjsefnie Nit (optional)
Line 29: sum(rel / _log2(i + 2) ...) evaluates _log2(i + 2) repeatedly. Could memoize _log2 for tiny perf gain, but this is an eval-time metric over small-k lists; not worth the complexity.

plz fix it

@himanshu231204

Copy link
Copy Markdown
Member

/oc work on Nit (optional)

@opencode-agent

Copy link
Copy Markdown
Contributor

remote: Permission to Nitjsefnie-OSC/openagent-eval.git denied to opencode-agent[bot].
fatal: unable to access 'https://github.com/Nitjsefnie-OSC/openagent-eval.git/': The requested URL returned error: 403

opencode session  |  github run

@himanshu231204
himanshu231204 merged commit d093f04 into OpenAgentHQ:main Jul 23, 2026
8 checks passed
@github-actions

Copy link
Copy Markdown

🎉 Congratulations @Nitjsefnie!

Your pull request has been successfully merged into main. 🚀

Thank you for contributing to OpenAgentHQ and helping improve the project.

We truly appreciate your contribution and hope to see you back with more amazing PRs!

Happy Open Sourcing! ❤️

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.

metrics(ndcg): DCG discount is off by one — a relevant doc at rank 2 scores a perfect 1.0

2 participants