Gnalbandyan/add physics

[gnalbandyan]

Added PHYSICS benchmark, updated Scientific Knowledge documentation page

Summary by CodeRabbit

• Documentation

  • Consolidated scientific-knowledge docs into a compact dataset overview table and updated evaluation examples to new parameter conventions.

• New Features

  • Added physics evaluation support: dataset preparation and export, specialized scoring for physics answers, and new prompt templates for problem generation and judgement.

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

EN/ZH split filenames swapped
In the final loop, zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"]) writes English examples to test.jsonl and Chinese examples to zh.jsonl, but then uses en_zh for the combined split. This makes test effectively EN-only and zh ZH-only, which seems fine, but contradicts the naming in the docs/config (EN default is called test). If test is intended to be the full test split, this is wrong; if test is intended to be EN-only, rename test to en (or update dataset defaults/docs) to avoid consumers accidentally evaluating the wrong language split.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new Physics dataset package (data prep and config), physics-specific metrics and judge/prompt configs, and updates scientific-knowledge docs to a compact dataset table and revised evaluation examples.

Changes

Cohort / File(s)	Summary
Physics Dataset nemo_skills/dataset/physics/__init__.py, nemo_skills/dataset/physics/prepare.py	New dataset module with evaluation/config constants and data-preparation utilities (strip_boxed, process_answer, format_entry, write_data_to_file, save_data). Produces JSONL splits (test en, zh, en_zh) from DESIMFJ Physics.
Metrics Registration & Implementation nemo_skills/evaluation/metrics/map_metrics.py, nemo_skills/evaluation/metrics/physics_metrics.py, nemo_skills/evaluation/metrics/math_metrics.py	Registers physics in METRICS_MAP; adds PhysicsMetrics subclass (is_correct_judgement, get_incorrect_sample) and exposes MathMetrics.is_correct_judgement wrapper used in scoring flow.
Prompt Configs nemo_skills/prompt/config/generic/physics.yaml, nemo_skills/prompt/config/judge/physics.yaml	Adds generic problem prompt enforcing LaTeX and boxed answers, and judge prompt for producing [Correct]/[Incorrect] judgements with handling rules for multiple answers and equivalence.
Docs docs/evaluation/scientific-knowledge.md, docs/evaluation/index.md, docs/index.md	Converts narrative benchmark sections into a consolidated dataset overview table and simplifies example listings; updates example evaluation snippets and parameter names.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Prep as DataPrep
    participant Storage as Dataset (JSONL)
    participant Evaluator as PhysicsMetrics
    participant Judge as JudgePrompt
    participant Model as JudgeModel

    User->>Prep: load_dataset(DESIMFJ Physics)
    Prep->>Prep: strip_boxed / process_answer / format_entry
    Prep->>Storage: write JSONL splits (en, zh, en_zh)
    User->>Evaluator: submit prediction(s)
    Evaluator->>Judge: craft judge prompt (problem, generation, expected_answer)
    Judge->>Model: send prompt to judge model/server
    Model-->>Judge: return judgement ([Correct]/[Incorrect])
    Judge-->>Evaluator: judgement text
    Evaluator-->>User: score dict (judge_correct, metrics)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Audio input output integration #1157 — Also modifies METRICS_MAP to add a new metrics entry; likely touches the same registration area.
• Add RULERv2 #1106 — Similar METRICS_MAP extension pattern (adds a metrics class registration).
• Added AAI-Omniscience Benchmark #1161 — Adds new dataset package and metrics module with registration, matching this PR's scaffolding.

Suggested labels

enhancement

Suggested reviewers

• ekmb
• Kipok

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Gnalbandyan/add physics' is overly vague and uses a branch naming convention rather than describing the actual changes; it does not clearly convey the main purpose of adding a Physics benchmark.	Use a more descriptive title like 'Add Physics benchmark with dataset, metrics, and evaluation configuration' to clearly summarize the main changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch gnalbandyan/add_physics

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]

Actionable comments posted: 7

🤖 Fix all issues with AI agents

In `@docs/evaluation/scientific-knowledge.md`:
- Around line 10-13: Remove the invalid HTML break tags in the markdown table
and following code block by replacing all occurrences of "<br>" and "</br>" with
valid markdown-friendly breaks (e.g., use "<br/>" if HTML breaks are required,
or convert to plain newlines/Markdown line breaks) in the rows containing
"**GPQA**", "**SciCode**" and the surrounding table lines (also fix the similar
instances around lines 40-40 referenced). Ensure table cells remain properly
formatted after the change.
- Around line 5-17: Add a new "Physics benchmark" subsection below the "Dataset
Overview" table that targets the "Physics" row: include the example evaluation
command (CLI or script) to run the benchmark, the expected baseline
results/metrics to compare against, model-testing details (prompt format,
scoring/judging rules and any automated judge used), and dataset-specific notes
describing the EN/ZH splits and how to select the EN split for evaluation;
reference the "Physics" dataset name from the table and ensure the subsection
succinctly documents command, expected results, model testing, and
dataset-specific setup (EN/ZH selection and judge configuration).

In `@nemo_skills/dataset/physics/__init__.py`:
- Around line 15-18: The inline comment next to METRICS_TYPE is incorrect:
update the comment to reflect that METRICS_TYPE = "physics" uses the
PhysicsMetrics class (not MathMetrics) and still sets compute_no_answer=False;
modify the comment on the METRICS_TYPE line accordingly and ensure surrounding
constants DATASET_GROUP, METRICS_TYPE, and GENERATION_ARGS remain unchanged.

In `@nemo_skills/dataset/physics/prepare.py`:
- Line 68: Change the zip call to enforce that the two iterables have identical
lengths by adding strict=True to the zip invocation used in the loop over
eng_data, ch_data, full_data and ["test", "zh", "en_zh"], i.e., update the for
loop that binds split_data and split_name so zip(..., strict=True) is used
instead of a plain zip to ensure mismatched lengths raise an error.
- Around line 29-32: Add strict=True to the zip(...) invocation used to pair the
parallel lists in this module — locate the zip call in this file (the one that
pairs items when building examples, adjacent to process_answer) and change
zip(a, b) to zip(a, b, strict=True) so mismatched lengths raise immediately;
ensure the call site where the pairing logic is implemented is updated (the zip
used inside the example-building function in this file).

In `@nemo_skills/prompt/config/generic/physics.yaml`:
- Around line 2-5: The YAML prompt contains a typo in the third rule: change the
word "seperated" to the correct spelling "separate" in the instruction that
reads "If there are multiple final answers, please seperated them by commas in
\\boxed{{}}"; update that sentence so it reads "If there are multiple final
answers, please separate them by commas in \\boxed{{}}", keeping the surrounding
LaTeX guidance and formatting intact (file:
nemo_skills/prompt/config/generic/physics.yaml; locate the rule text containing
"seperated").

In `@nemo_skills/prompt/config/judge/physics.yaml`:
- Around line 16-17: The prompt string fragment "Question: {problem}, Output
sentence: {generation}, Correct answer: {expected_answer}, Judge- ment:" is
broken by a hyphenated line break; fix it by merging the split token into a
single word and line so it reads "Judgement:" (i.e., replace "Judge- ment:" with
"Judgement:") ensuring the entire prompt line is contiguous: "Question:
{problem}, Output sentence: {generation}, Correct answer: {expected_answer},
Judgement:".

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add Physics benchmark details (command, expected results, model testing, dataset-specific notes).
The table introduces Physics but there’s no physics-specific example command, expected results, or dataset notes (e.g., EN/ZH splits, judge setup). Please add a short subsection covering these items.
As per coding guidelines: When adding new benchmarks, add documentation with example commands, expected results, model testing details, and dataset-specific information.

🤖 Prompt for AI Agents

In `@docs/evaluation/scientific-knowledge.md` around lines 5 - 17, Add a new
"Physics benchmark" subsection below the "Dataset Overview" table that targets
the "Physics" row: include the example evaluation command (CLI or script) to run
the benchmark, the expected baseline results/metrics to compare against,
model-testing details (prompt format, scoring/judging rules and any automated
judge used), and dataset-specific notes describing the EN/ZH splits and how to
select the EN split for evaluation; reference the "Physics" dataset name from
the table and ensure the subsection succinctly documents command, expected
results, model testing, and dataset-specific setup (EN/ZH selection and judge
configuration).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix invalid <br> tags in the table and after the code block.

🛠️ Suggested fix

-| **GPQA** | 448 (main)<br>198 (diamond)</br>546 (ext.) | MCQ (4) | Physics, Chemistry, Biology | No | diamond | [HF](https://huggingface.co/datasets/Idavidrein/gpqa) |
+| **GPQA** | 448 (main)<br>198 (diamond)<br>546 (ext.) | MCQ (4) | Physics, Chemistry, Biology | No | diamond | [HF](https://huggingface.co/datasets/Idavidrein/gpqa) |
-| **SciCode** | 80</br>(338 subtasks) | Code gen | Scientific computing | No | test+val | [HF](https://huggingface.co/datasets/SciCode1/SciCode) |
+| **SciCode** | 80<br>(338 subtasks) | Code gen | Scientific computing | No | test+val | [HF](https://huggingface.co/datasets/SciCode1/SciCode) |
-</br>
+<br>

Also applies to: 40-40

🤖 Prompt for AI Agents

In `@docs/evaluation/scientific-knowledge.md` around lines 10 - 13, Remove the
invalid HTML break tags in the markdown table and following code block by
replacing all occurrences of "<br>" and "</br>" with valid markdown-friendly
breaks (e.g., use "<br/>" if HTML breaks are required, or convert to plain
newlines/Markdown line breaks) in the rows containing "**GPQA**", "**SciCode**"
and the surrounding table lines (also fix the similar instances around lines
40-40 referenced). Ensure table cells remain properly formatted after the
change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Comment mismatch: METRICS_TYPE uses PhysicsMetrics, not MathMetrics.

🛠️ Suggested fix

-METRICS_TYPE = "physics"  # This uses the MathMetrics class, but with compute_no_answer=False
+METRICS_TYPE = "physics"  # Uses PhysicsMetrics (compute_no_answer defaults to False)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# settings that define how evaluation should be done by default (all can be changed from cmdline)
	DATASET_GROUP = "math"
	METRICS_TYPE = "physics" # This uses the MathMetrics class, but with compute_no_answer=False
	GENERATION_ARGS = "++prompt_config=generic/physics ++eval_type=math"
	# settings that define how evaluation should be done by default (all can be changed from cmdline)
	DATASET_GROUP = "math"
	METRICS_TYPE = "physics" # Uses PhysicsMetrics (compute_no_answer defaults to False)
	GENERATION_ARGS = "++prompt_config=generic/physics ++eval_type=math"

🤖 Prompt for AI Agents

In `@nemo_skills/dataset/physics/__init__.py` around lines 15 - 18, The inline
comment next to METRICS_TYPE is incorrect: update the comment to reflect that
METRICS_TYPE = "physics" uses the PhysicsMetrics class (not MathMetrics) and
still sets compute_no_answer=False; modify the comment on the METRICS_TYPE line
accordingly and ensure surrounding constants DATASET_GROUP, METRICS_TYPE, and
GENERATION_ARGS remain unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's view the full prepare.py file to understand the context
find . -type f -name "prepare.py" -path "*/physics/*" | head -5

Repository: NVIDIA-NeMo/Skills

Length of output: 102

---

🏁 Script executed:

# List files in the physics dataset directory
fd "prepare.py" --path "*physics*"

Repository: NVIDIA-NeMo/Skills

Length of output: 277

---

🏁 Script executed:

# Search for the exact file
find . -type f -name "prepare.py" | grep physics

Repository: NVIDIA-NeMo/Skills

Length of output: 102

---

🌐 Web query:

HuggingFace dataset desimfj/PHYSICS schema answer field format

💡 Result:

In desimfj/PHYSICS, the answer field is a nested list: List[List[str]] (HF viewer shows it as a sequence of sequences). Each outer list element corresponds to one sub-question, and the inner list contains one or more accepted answer strings (typically LaTeX, often wrapped with \\boxed{...}).[1]

Example patterns from the dataset:[1]

• Single sub-question:
  answer = [[ "\\boxed{3}" ]]
• Two sub-questions:
  answer = [[ "\\boxed{-60}" ], [ "\\boxed{4}" ]]
• MCQ:
  answer = [[ "\\boxed{B}" ]]

---

🏁 Script executed:

# Search for references to this dataset or answer processing
rg "desimfj/PHYSICS" --type py

Repository: NVIDIA-NeMo/Skills

Length of output: 154

---

🏁 Script executed:

# Look for load_dataset calls related to physics
rg "load_dataset.*physics" --type py -i

Repository: NVIDIA-NeMo/Skills

Length of output: 154

---

🏁 Script executed:

# Now let's read the full prepare.py file to see the context and line 68
cat -n ./nemo_skills/dataset/physics/prepare.py

Repository: NVIDIA-NeMo/Skills

Length of output: 2866

---

🏁 Script executed:

# Check for Python version requirements
find . -name "setup.py" -o -name "pyproject.toml" -o -name ".python-version" | xargs cat 2>/dev/null | head -50

Repository: NVIDIA-NeMo/Skills

Length of output: 1986

---

🏁 Script executed:

# Also check for requirements files
find . -name "requirements*.txt" | xargs head -5 2>/dev/null | head -30

Repository: NVIDIA-NeMo/Skills

Length of output: 174

---

🏁 Script executed:

# Search for python_requires in setup files
rg "python_requires|python =" --type py --type toml | head -10

Repository: NVIDIA-NeMo/Skills

Length of output: 220

---

🏁 Script executed:

# Verify zip behavior and confirm both lists have same length
python3 << 'EOF'
# Simulating the actual data
eng_data = [1, 2, 3]  # placeholder for actual data
ch_data = [1, 2, 3]
full_data = [1, 2, 3]

split_names = ["test", "zh", "en_zh"]
split_datas = [eng_data, ch_data, full_data]

# Check lengths match
print(f"Length of split_datas: {len(split_datas)}")
print(f"Length of split_names: {len(split_names)}")
print(f"Balanced: {len(split_datas) == len(split_names)}")

# Test that zip(strict=True) would work (Python 3.10+)
result = list(zip(split_datas, split_names, strict=True))
print(f"zip(strict=True) works: {len(result)} pairs")
EOF

Repository: NVIDIA-NeMo/Skills

Length of output: 158

---

Add strict=True to the zip() call at line 68 for safety.

The nested comprehension in process_answer() is correct—the dataset schema confirms answer is List[List[str]], and the code handles it properly with no risk of data corruption. However, line 68 should use zip(..., strict=True) to catch potential future bugs if the list lengths ever become unequal.

Fix for line 68

-    for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"]):
+    for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"], strict=True):

🤖 Prompt for AI Agents

In `@nemo_skills/dataset/physics/prepare.py` around lines 29 - 32, Add strict=True
to the zip(...) invocation used to pair the parallel lists in this module —
locate the zip call in this file (the one that pairs items when building
examples, adjacent to process_answer) and change zip(a, b) to zip(a, b,
strict=True) so mismatched lengths raise immediately; ensure the call site where
the pairing logic is implemented is updated (the zip used inside the
example-building function in this file).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check Python version requirements
echo "=== Python version requirements ==="
rg -n "python_requires|requires-python|python_version" -S -i
echo ""
echo "=== pyproject.toml content (relevant sections) ==="
if [ -f pyproject.toml ]; then
  head -50 pyproject.toml
fi

Repository: NVIDIA-NeMo/Skills

Length of output: 1818

---

🏁 Script executed:

#!/bin/bash
# Check for Ruff configuration and B905 rule
echo "=== Looking for Ruff configuration ==="
find . -maxdepth 2 -type f \( -name "ruff.toml" -o -name ".ruff.toml" -o -name "pyproject.toml" \) -exec echo "File: {}" \; -exec cat {} \;

Repository: NVIDIA-NeMo/Skills

Length of output: 3330

---

🏁 Script executed:

#!/bin/bash
# Verify the actual zip call and surrounding context
echo "=== Context around line 68 in prepare.py ==="
sed -n '60,75p' nemo_skills/dataset/physics/prepare.py

Repository: NVIDIA-NeMo/Skills

Length of output: 508

---

Reconsider the strict= parameter value for alignment with project philosophy.

Ruff B905 requires explicit strict= parameter in zip(). However, the suggested fix of strict=False contradicts the project's coding guidelines about explicit error handling and letting errors surface rather than silently failing. Since both iterables are guaranteed to have exactly 3 elements each, use strict=True instead to enforce that expectation:

-    for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"]):
+    for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"], strict=True):

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"]):
	for split_data, split_name in zip([eng_data, ch_data, full_data], ["test", "zh", "en_zh"], strict=True):

🧰 Tools

🪛 Ruff (0.14.14)

[warning] 68-68: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

🤖 Prompt for AI Agents

In `@nemo_skills/dataset/physics/prepare.py` at line 68, Change the zip call to
enforce that the two iterables have identical lengths by adding strict=True to
the zip invocation used in the loop over eng_data, ch_data, full_data and
["test", "zh", "en_zh"], i.e., update the for loop that binds split_data and
split_name so zip(..., strict=True) is used instead of a plain zip to ensure
mismatched lengths raise an error.

[jiacheng-xu]

Source of the prompt?

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

incomplete prompt, text cut off at "Judge-ment:" instead of proper instruction

Suggested change

	Question: {problem}, Output sentence: {generation}, Correct answer: {expected_answer}, Judge-
	ment:
	Question: {problem}, Output sentence: {generation}, Correct answer: {expected_answer}. Judgement:

[greptile-apps]

EVAL_SPLIT = "test" creates naming confusion - per prepare.py:68, test contains only EN examples (1000), but doc table (line 15) says default is "EN" with 1,000 examples, suggesting alignment. However, the file labeling is confusing: test.jsonl = EN-only, zh.jsonl = ZH-only, en_zh.jsonl = combined. Consider renaming test to en for clarity or update docs to explicitly state that "test" = "EN-only split"

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[jiacheng-xu]

lgtm

[Kipok]

please fix these issues reported by mkdocs

INFO    -  Doc file 'index.md' contains a link './evaluation/scientific-knowledge.md#hle', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#hle'.
INFO    -  Doc file 'index.md' contains a link './evaluation/scientific-knowledge.md#scicode', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#scicode'.
INFO    -  Doc file 'index.md' contains a link './evaluation/scientific-knowledge.md#gpqa', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#gpqa'.
INFO    -  Doc file 'evaluation/index.md' contains a link './scientific-knowledge.md#hle', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#hle'.
INFO    -  Doc file 'evaluation/index.md' contains a link './scientific-knowledge.md#scicode', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#scicode'.
INFO    -  Doc file 'evaluation/index.md' contains a link './scientific-knowledge.md#gpqa', but the doc 'evaluation/scientific-knowledge.md'
           does not contain an anchor '#gpqa'.

[Kipok]

also the table is a bit too wide - have to scroll through. Maybe we can reorganize to reduce number of columns? E.g. link can just be fused into the first column. And if we also remove images (can just add a footnote maybe for hle), then it's going to fit

[gnalbandyan]

fixed these, for the images column, we plan to add multimodal data soon, thats why its there

[Kipok]

do we need ++inference.extra_body.skip_special_tokens=false ?

[Kipok]

the table seems to have fewer datasets than in the original docs. E.g. mmlu-redux is missing? Also scicode section had some useful extra details which are good to keep?

[gnalbandyan]

Added mmlu-redux, the scicide note was about gpt-oss, which is now removed.

[Kipok]

Can we add expected results to all of these commands? You can use mkdocs dropdowns / tabs to make it use fewer space, e.g. can have a toggle per benchmark / evaluation mode or something. But having reference numbers is useful

[Kipok]

you most likely want this to be \boxed, not \\boxed. With |- syntax yaml doesn't need \ escaping, so this will render as \\

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Documentation table states default is "EN", but __init__.py:19 uses EVAL_SPLIT = "test" which maps to EN-only split per prepare.py:68. While technically aligned (both refer to 1,000 EN examples), consider clarifying by either updating table from "EN" to "test" for consistency with code, or renaming test.jsonl to en.jsonl in prepare.py:68 and updating EVAL_SPLIT = "en" for better semantic clarity. Current naming creates confusion since test typically implies the full test set, not a language-specific subset.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Missing space after "Judgement:" - judge will append its response directly without separation

Suggested change

	Question: {problem}, Output sentence: {generation}, Correct answer: {expected_answer}, Judgement:
	Question: {problem}, Output sentence: {generation}, Correct answer: {expected_answer}, Judgement:

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[Kipok]

you can skip the init if it's identical to parent class

[gnalbandyan]

@Kipok the defaults are different. I could remove it and use a partial in map_metrics.py, but it looks more explicit and straightforward to me having the defaults here.

[greptile-apps]

_{10 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Incorrect super().__init__ args
PhysicsMetrics.__init__ accepts answer_key but doesn’t pass it to MathMetrics.__init__, so MathMetrics.question_key/answer_key stay at their defaults (problem/predicted_answer). This will break evaluation when predictions use the expected generation key (e.g., pass@k/majority@k will look up predicted_answer and raise KeyError). Pass answer_key (and any non-default question_key if needed) through to super().__init__ instead of only setting self.answer_key.

[greptile-apps]

Wrong dataset group/type
DATASET_GROUP = "math" and GENERATION_ARGS sets ++eval_type=math, but this PR introduces a physics-specific prompt/metrics. Using the math group/type here can route PHYSICS runs through the wrong dataset category/config defaults and can select the wrong evaluation pipeline settings.

If this benchmark is meant to show up under scientific knowledge (per docs) and be evaluated with the physics metrics, the dataset metadata should be consistent with that (group + eval_type).

[greptile-apps]

Judge prompt unparseable
The prompt ends with a stray "] after Judgement:. This will be included in the model input and makes the expected output format ambiguous; it also looks like an accidental truncation/quoting artifact. Remove the extra characters so the judge sees a clean instruction ending at Judgement: (with an appropriate trailing space/newline).

[greptile-apps]

Prompt typo affects instruction
Rule 3 says “please seperated them by commas” (typo). This gets copied into the model’s instructions and can hurt prompt clarity / consistency across benchmarks. Fix to “separate”.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@docs/evaluation/scientific-knowledge.md`:
- Around line 77-82: The multiline string passed into ctx=wrap_arguments(...)
contains an unintended blank line (a stray newline) between the
"++parse_reasoning=True " and "++tool_modules=..." lines which triggers the
markdownlint indented-code-block warning; edit the argument to wrap_arguments
(the ctx=wrap_arguments(...) call) and remove the blank line so the
configuration lines are contiguous within the string (no extra empty line),
preserving existing spacing and quotes.

In `@nemo_skills/evaluation/metrics/physics_metrics.py`:
- Around line 29-39: The return type hint for is_correct_judgement is incorrect
because it can return None when return_none=True or the judgement format is
unrecognized; update the signature of is_correct_judgement to reflect
Optional[bool] (or Union[bool, None]) and add the necessary typing import (e.g.,
Optional) to the module so the annotation matches behavior and aligns with
utils.is_correct_judgement.

🧹 Nitpick comments (3)

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

84-86: Signature mismatch with subclass override.

PhysicsMetrics.is_correct_judgement adds a return_none parameter that this base method doesn't accept. While not breaking today (callers don't pass return_none), this inconsistency could cause TypeError if someone calls the method polymorphically with return_none=True on a MathMetrics instance. Consider adding return_none: bool = False here too for a consistent interface.

Suggested fix

-    def is_correct_judgement(self, judgement: str) -> bool:
+    def is_correct_judgement(self, judgement: str, return_none: bool = False) -> bool:
         """Check if the judgement is correct."""
-        return is_correct_judgement(judgement)
+        return is_correct_judgement(judgement, return_none=return_none)

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

25-27: Pass answer_key through to super().__init__ instead of overriding after the fact.

MathMetrics.__init__ already accepts answer_key. Passing it through avoids the redundant set-then-override pattern:

Suggested fix

     def __init__(self, compute_no_answer: bool = False, answer_key: str = "generation"):
-        super().__init__(compute_no_answer=compute_no_answer)
-        self.answer_key = answer_key
+        super().__init__(compute_no_answer=compute_no_answer, answer_key=answer_key)

docs/index.md (1)

20-20: Consider mentioning physics in the example list and adding anchor links for consistency.

This PR adds the Physics benchmark, but the example list here says "hle, scicode, gpqa" without mentioning physics. Also, every other category line links individual benchmarks to their doc anchors, while this one uses plain text.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Stray blank line inside function call.

Line 81 is blank inside the ctx=wrap_arguments(...) string, which causes the markdownlint indented-code-block warning and looks unintentional in the example.

Suggested fix

         "++chat_template_kwargs.enable_thinking=true ++parse_reasoning=True "
         "++tool_modules=[nemo_skills.mcp.servers.python_tool::PythonTool] "
-
     ),

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	"++inference.temperature=0.6 ++inference.top_p=0.95 "
	"++inference.tokens_to_generate=131072 "
	"++chat_template_kwargs.enable_thinking=true ++parse_reasoning=True "
	"++tool_modules=[nemo_skills.mcp.servers.python_tool::PythonTool] "
	),
	"++inference.temperature=0.6 ++inference.top_p=0.95 "
	"++inference.tokens_to_generate=131072 "
	"++chat_template_kwargs.enable_thinking=true ++parse_reasoning=True "
	"++tool_modules=[nemo_skills.mcp.servers.python_tool::PythonTool] "
	),

🧰 Tools

🪛 markdownlint-cli2 (0.20.0)

[warning] 79-79: Code block style
Expected: fenced; Actual: indented

(MD046, code-block-style)

🤖 Prompt for AI Agents

In `@docs/evaluation/scientific-knowledge.md` around lines 77 - 82, The multiline
string passed into ctx=wrap_arguments(...) contains an unintended blank line (a
stray newline) between the "++parse_reasoning=True " and "++tool_modules=..."
lines which triggers the markdownlint indented-code-block warning; edit the
argument to wrap_arguments (the ctx=wrap_arguments(...) call) and remove the
blank line so the configuration lines are contiguous within the string (no extra
empty line), preserving existing spacing and quotes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Return type hint -> bool is inaccurate — method can return None.

When return_none=True and the judgement format is unrecognized, this returns None. The hint should reflect that:

-    def is_correct_judgement(self, judgement: str, return_none: bool = False) -> bool:
+    def is_correct_judgement(self, judgement: str, return_none: bool = False) -> bool | None:

Note: the same inaccuracy exists in utils.is_correct_judgement (which uses Union[bool, None] correctly in its signature), so this would bring them into alignment.

🤖 Prompt for AI Agents

In `@nemo_skills/evaluation/metrics/physics_metrics.py` around lines 29 - 39, The
return type hint for is_correct_judgement is incorrect because it can return
None when return_none=True or the judgement format is unrecognized; update the
signature of is_correct_judgement to reflect Optional[bool] (or Union[bool,
None]) and add the necessary typing import (e.g., Optional) to the module so the
annotation matches behavior and aligns with utils.is_correct_judgement.