Repository navigation
Count invalid event-verification answers as abstentions - #350
sylvesterkaczmarek wants to merge 2 commits into
Conversation
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
pengcuo
left a comment
There was a problem hiding this comment.
Review notes. The logic is correct: I traced the parametrized cases (e.g. truth yes,no,no / pred yes,?,? -> yes F1 1.0, no F1 0.0 -> 0.5), and labels= without the sentinel does count those rows as false negatives.
-
This changes the headline score, but the docstring still says it matches the paper.
evaluate()still saysmacro_f1"matches VANTAGE paper Table 2". Before, unparseable answers were dropped. Now they lower recall, so the same predictions score lower whenever any answer fails to parse. Results from before and after this PR are not comparable. Either confirm that Table 2 counts parse failures as abstentions and say so in the docstring, or keep the old number under its own key. Otherwise people will compare new runs with published numbers. -
The output columns now depend on the data. Because
labelsomits the sentinel, sklearn emitsmicro avginstead ofaccuracyonly when some answer is invalid. The code then overwritesaccuracy, so those runs get bothaccuracyandmicro avg--*, and fully valid runs get onlyaccuracy. Themicro avgvalues also ignore the abstained rows (precision/recall over in-label predictions only), which is misleading next toaccuracy. Dropmicro avgfrom the report when the sentinel is present, or compute precision/recall/F1 from the same all-rows basis. The tests only compare againstclassification_reportfor fully valid inputs, so this is not covered. -
Low: a matched ground-truth row with a NaN answer becomes the class
'nan'.if pd.isna(gt): continueonly guards thegt_row is Nonebranch. A NaNgt_row['answer']becomesstr(gt)='nan', andlabelsnow includesnanin the macro average. This already happened for valid predictions, but with abstentions kept it now also applies to unparseable ones. A one-lineisnacheck after the lookup fixes both cases. -
Low: the tests need
-c /dev/null. The PR description says the focused run needs-c /dev/null --confcutdir=tests, so they are not collectable under the repo's pytest config. Please either make them run normally in CI or state where they run.
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
|
Thanks, @pengcuo. I addressed the report consistency and documentation points in signed commit |
Resubmits NVIDIA/cosmos#365 at the new Cosmos Framework evaluation path, as requested by @yaoxu-crypto after the evaluation suites moved under
.agents/skills/. The original change addresses NVIDIA/cosmos#364.Correction
Valid Predictionsas parse coverage and skip NaN ground truth even when it came from a matched annotation row.Metric comparability
The abstention-aware
macro_f1is not directly comparable with legacy reports that discarded invalid responses. The docstring no longer claims the published VANTAGE Table 2 numbers were computed using this policy; that historical methodology has not been verified.Verification
python -m py_compileandgit diff --checkpassed.Focused command from
.agents/skills/evaluation/cosmos3/reasoner/vlmevalkitwith the CPU dependencies installed:The same focused invocation is documented in
tests/README.md. Using-c /dev/nullkeeps this vendored evaluator unit run independent of the framework root’s broader pytest configuration. These tests are not claimed to run in the root CI workflow. No video downloading, model inference, GPU workloads, or full benchmark runs were performed. No runtime dependencies were changed.Original PR: NVIDIA/cosmos#365. DCO sign-off is included. Thanks for the migration instructions.