Fix no_answer metric overcounting in _compute_pass_at_k

[sgunasekar]

Issue:

• The no_answer accumulation was overcounting as it inside the for-score_method loop, causing it to be incremented once per score method per sample instead of once per sample.
• With N score methods (e.g. symbolic_correct, judge_correct, both_correct, any_correct), no_answer was overcounted by N×, producing impossible values > 100%.

Fix:

• Move no_answer accumulation outside the score_method loop into its own for-k loop\

Summary by CodeRabbit

• Refactor
  • Reordered internal evaluation flow for pass@k and no-answer handling so per-k calculations and updates occur in a single outer loop. This changes internal metric update sequencing while preserving external behavior and public interfaces. Improved predictability of metric computation ordering; no user-facing API changes.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Moves pass@k no-answer computation and metric update out of the inner per-score-method loop into an outer loop over k (1..len(predictions)); for each k it computes no_answer, updates the pass@k no-answer metric, then calls _update_metrics_for_pass for that k.

Changes

Cohort / File(s)	Summary
Pass@k Metric Refactor nemo_skills/evaluation/metrics/base.py	Reorganized control flow: iterate k from 1..len(predictions) in an outer loop, compute no_answer and update the pass@k no-answer metric per k, then call _update_metrics_for_pass(k). Removes per-score-method placement of these updates. (+11/-11)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and accurately describes the main fix in the changeset: moving no_answer accumulation out of the score method loop to prevent overcounting, matching the issue and solution described in the PR objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/no-answer-overcounting

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

nemo_skills/evaluation/metrics/base.py (1)

413-418: _update_metrics_for_pass is called inside the score_method loop, contradicting its docstring.

The method's docstring (line 341–347) explicitly states it should be called "one time after the loop over all score_method" for metrics that do not depend on the correctness method. However, the call at lines 413–418 is indented inside both the for score_method loop (line 369) and the for k loop (line 382), meaning it executes N_score_methods × N_predictions times per sample instead of once.

No subclass currently overrides _update_metrics_for_pass, so this is a latent bug. However, if a future subclass adds an override to track score-independent metrics (e.g., token-level stats), it will silently overcount by a factor of N_score_methods. The no_answer metric block (lines 420+) demonstrates the correct structure—already positioned outside the score_method loop.

Move the _update_metrics_for_pass call outside the score_method loop to align with the docstring and prevent latent overcounting:

♻️ Proposed refactor

     for score_method in score_dicts[0].keys():
         ...
         for k in range(1, len(predictions) + 1):
             ...
             self._update_score_metrics_for_pass(
                 eval_dict=eval_dict,
                 k=k,
                 score_method=score_method,
                 score_dicts=score_dicts,
                 pass_score=instance_pass_score,
                 predictions=predictions,
                 predicted_answers=predicted_answers,
             )
-
-            self._update_metrics_for_pass(
-                eval_dict=eval_dict,
-                k=k,
-                predictions=predictions,
-                predicted_answers=predicted_answers,
-            )
+
+    for k in range(1, len(predictions) + 1):
+        self._update_metrics_for_pass(
+            eval_dict=eval_dict,
+            k=k,
+            predictions=predictions,
+            predicted_answers=predicted_answers,
+        )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemo_skills/evaluation/metrics/base.py` around lines 413 - 418, The call to
_update_metrics_for_pass is incorrectly inside the for score_method loop causing
it to run once per score method; move the
_update_metrics_for_pass(eval_dict=eval_dict, k=k, predictions=predictions,
predicted_answers=predicted_answers) call out of the for score_method loop so it
is executed a single time after all score_method iterations (keep it at the same
level as the existing no_answer metric block so it still runs per-k/pass and
continues to have access to eval_dict, k, predictions, and predicted_answers).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@nemo_skills/evaluation/metrics/base.py`:
- Around line 413-418: The call to _update_metrics_for_pass is incorrectly
inside the for score_method loop causing it to run once per score method; move
the _update_metrics_for_pass(eval_dict=eval_dict, k=k, predictions=predictions,
predicted_answers=predicted_answers) call out of the for score_method loop so it
is executed a single time after all score_method iterations (keep it at the same
level as the existing no_answer metric block so it still runs per-k/pass and
continues to have access to eval_dict, k, predictions, and predicted_answers).

[Kipok]

nice, thanks! This is most likely why we had this issue before #715

[Kipok]

not related to this pr, but I guess this needs to be moved out of the loop as well?

[sgunasekar]

is this referring to _update_metrics_for_pass?

[sgunasekar]

@Kipok - added this fix - could you do another quick review before we merge this.

[Kipok]

and ideally we can modify one of the test files in test_metrics.py to trigger this code path, so that this is covered by tests in the future. But feel free to merge as is and we can fix those in a separate PR

[coderabbitai]

🧹 Nitpick comments (1)

nemo_skills/evaluation/metrics/base.py (1)

413-423: Add test coverage for the no_answer / pass@k code path.

No test currently exercises this fixed path (acknowledged by reviewer Kipok). Without a regression test the overcount can silently reappear. The test should construct predictions where predicted_answers contains None values across multiple score methods and assert no_answer in each pass@{k} bucket equals exactly 1.0 × percentage (not N × percentage).

Run the following to check existing test coverage for this path:

#!/bin/bash
# Check for any existing tests covering no_answer in pass@k context
rg -n "no_answer" --type=py -A3 -B3

# Find the test_metrics.py file mentioned in the PR comments
fd "test_metrics.py" --type=f

Would you like me to draft a regression test for test_metrics.py covering the no_answer overcounting scenario, or open a new issue to track this?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemo_skills/evaluation/metrics/base.py` around lines 413 - 423, The pass@k
loop over predicted answers is not covered by tests and can overcount no-answer
rates; add a regression test that constructs multiple scored prediction lists
where predicted_answers contains None values and asserts that for each k the
eval_dict[f"pass@{k}"]["no_answer"] equals the correct percentage (1.0 ×
percentage) rather than N×percentage; specifically exercise the code path using
compute_no_answer=True and call the logic that runs the loop (the code that
invokes predicted_answers, the pass@k loop, and _update_metrics_for_pass) to
verify eval_dict entries for pass@{k} and pass@1[avg-of-{k}] match expected
values across different k and scoring methods.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@nemo_skills/evaluation/metrics/base.py`:
- Around line 413-423: The pass@k loop over predicted answers is not covered by
tests and can overcount no-answer rates; add a regression test that constructs
multiple scored prediction lists where predicted_answers contains None values
and asserts that for each k the eval_dict[f"pass@{k}"]["no_answer"] equals the
correct percentage (1.0 × percentage) rather than N×percentage; specifically
exercise the code path using compute_no_answer=True and call the logic that runs
the loop (the code that invokes predicted_answers, the pass@k loop, and
_update_metrics_for_pass) to verify eval_dict entries for pass@{k} and
pass@1[avg-of-{k}] match expected values across different k and scoring methods.

[coderabbitai]

🧹 Nitpick comments (1)

nemo_skills/evaluation/metrics/base.py (1)

413-417: Add a regression test for the no_answer overcounting fix.

As noted in the review discussion, there is no test covering this code path. A test with >1 score method (e.g., symbolic_correct + judge_correct) should assert that no_answer in pass@{k} and pass@1[avg-of-{k}] stays ≤ 100% and equals the expected per-sample value rather than a multiple of it.

Would you like me to draft a regression test for test_metrics.py that reproduces the original overcounting bug and verifies the fix, so this can be merged as a follow-up?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemo_skills/evaluation/metrics/base.py` around lines 413 - 417, Add a
regression test in test_metrics.py that reproduces the original overcounting by
calling the metric computation code path that uses predicted_answers and
predictions (the loop that updates eval_dict[f"pass@{k}"]["no_answer"] and
eval_dict[f"pass@1[avg-of-{k}]"]["no_answer"]) with multiple scoring methods
(e.g., symbolic_correct and judge_correct); construct a small dataset with one
or more samples where predicted_answers contains some None entries and assert
for each k that eval_dict["pass@{k}"]["no_answer"] <= 1.0 and
eval_dict["pass@1[avg-of-{k}]"]["no_answer"] equals the expected per-sample
average (not a multiplied value), thereby confirming the fix prevents
overcounting when multiple score methods are present.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@nemo_skills/evaluation/metrics/base.py`:
- Around line 413-417: Add a regression test in test_metrics.py that reproduces
the original overcounting by calling the metric computation code path that uses
predicted_answers and predictions (the loop that updates
eval_dict[f"pass@{k}"]["no_answer"] and
eval_dict[f"pass@1[avg-of-{k}]"]["no_answer"]) with multiple scoring methods
(e.g., symbolic_correct and judge_correct); construct a small dataset with one
or more samples where predicted_answers contains some None entries and assert
for each k that eval_dict["pass@{k}"]["no_answer"] <= 1.0 and
eval_dict["pass@1[avg-of-{k}]"]["no_answer"] equals the expected per-sample
average (not a multiplied value), thereby confirming the fix prevents
overcounting when multiple score methods are present.