Skip to content

[codex] add fold11 agent evaluation bundle#34

Draft
jmchandonia wants to merge 1 commit into
mainfrom
codex/fold11-agent-evaluation
Draft

[codex] add fold11 agent evaluation bundle#34
jmchandonia wants to merge 1 commit into
mainfrom
codex/fold11-agent-evaluation

Conversation

@jmchandonia

Copy link
Copy Markdown
Collaborator

Summary

  • add tracked fold-11 holdout-23 sub-agent artifacts, including action trace and generated outputs
  • add an evaluation report and machine-readable metrics against held-out gold
  • track heldout_harmonized.csv as heldout_harmonized.csv.gz because the raw CSV exceeds GitHub's 100 MB file limit; include a manifest with raw/compressed hashes

Evaluation Highlights

  • targeted forbidden-path scan found zero matches
  • agent produced 1,722,716 rows vs 912,417 gold rows
  • no keyed rows matched gold under evaluator key policy
  • generated JSON covered all 8/8 gold mapping categories

Validation

  • python -m py_compile evaluations/fold-11-holdout-23/agent_outputs/harmonize_heldout.py
  • targeted rg forbidden-path scan over evaluations/fold-11-holdout-23/AGENT_ACTION_LOG.md

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmchandonia's task —— View job


PR Review: fold-11 agent evaluation bundle

Tasks:

  • Gather context
  • Read key files
  • Review code quality
  • Review evaluation metrics
  • Post feedback

Summary

This PR adds the fold-11-holdout-23 evaluation bundle with all required artifacts. The evaluation records a complete agent run on held-out dataset 23 (ess-dive-a99be52b7a6114c-20230504T210134503379). The code and documentation are generally well-structured. Below are observations across the requested review dimensions.


Code Quality — harmonize_heldout.py

Positives:

  • Clean schema-first design with SCHEMA list (line 15) driving final output column selection.
  • Good use of pd.to_numeric(..., errors='coerce') throughout for safe numeric casting.
  • Timezone handling uses fixed offset Etc/GMT+7 with .tz_localize().tz_convert() which is correct for loggers recording in a fixed MST offset.

Issues / Suggestions:

  1. Hardcoded absolute paths (harmonize_heldout.py:9-11): RUN_DIR and RAW_DIR are machine-specific paths (/scratch/jmc/..., /h/jmc/...). The script will not run on any system other than the original author's machine. For reproducibility, these should either be parameterized (e.g., via argparse or environment variables) or documented clearly as intentional runtime paths.

  2. log_action not called for intermediate writes (harmonize_heldout.py:117): The log call covers the final CSV write, but intermediate steps (like the sensor merge or filtering) are not logged inline in the Python script itself. (These are logged externally in AGENT_ACTION_LOG.md, so this may be acceptable by design.)

  3. Replicate assignment is sensor-serial-based, not metadata-documented (harmonize_heldout.py:93-96): The replicate is assigned by cumcount() on sorted Sensor.SN within (site_id, depth_m). This is reasonable, but the mapping notes describe it as "sorted sensor serials" while the code sorts by ['site_id', 'depth_m', 'Sensor.SN'] before the groupby. The final merged replicate is joined back on Sensor.SN only (line 96), which would be incorrect if a single sensor appears in multiple (site_id, depth_m) groups — though in practice this may not occur given the data.

  4. Unused local_mst_to_utc_string function (harmonize_heldout.py:39-42): This function is defined but never called; the conversion is done inline at lines 99-100. Dead code should be removed.

  5. is_timeseries hardcoded to True without validation (harmonize_heldout.py:110): Since every row is classified as a timeseries record, this is appropriate, but the intent could be clearer with a comment noting the dataset is uniformly a continuous sensor record.

  6. interval_min for first row per group is NaN (harmonize_heldout.py:104-109): groupby.diff() produces NaN for the first observation in each series. This is a correct and expected behavior, but the downstream evaluation (evaluation_metrics.json:27, shared_row_value_matches.interval_min = "0/0") shows zero common rows, so this edge case was never tested against gold.


Evaluation Metrics

Key concern — 0% row precision and recall (evaluation_metrics.json:32-33):

The most notable result is row_precision: 0.0 and gold_recall: 0.0 with key_intersection: 0. The PR notes this is due to key_includes_dataset_index: false. This means the agent's output keys (site_id, datetime_UTC, depth_m, replicate) never matched any gold key — despite producing 1.7M rows vs. 912K gold rows.

This warrants investigation into why the keys don't match:

  • Row count discrepancy: Agent produced 1,722,716 rows vs. gold's 912,417 — nearly 2× more. Given the raw payload has 1,722,752 SWC rows and the agent excludes only 36, the agent output count is plausible. The gold output being ~53% of raw rows suggests the gold harmonizer applies more aggressive filtering.
  • Key mismatch: Even with differing row counts, zero key intersection across 912K gold keys is unexpected if the schemas are otherwise compatible. This suggests a systematic difference in how site_id or datetime_UTC is constructed, or a different timestamp timezone interpretation.

The EVALUATION.md correctly notes this should be "interpreted with the key policy" — but this is a significant signal that may be worth debugging. The transformation: 0/8 mismatch in JSON mapping also suggests the agent's transformation descriptions don't match gold verbatim.


JSON Mapping Quality

  • All 8/8 gold mapping categories are present — good coverage.
  • top_level_exact_matches: 5/7: The mismatches are location_metadata_files and sensor_metadata_files. The agent lists ["plot_metadata.csv", "sensor_metadata.csv"] for location_metadata_files but the gold apparently differs. This is a curation judgment call.
  • transformation: 0/8 exact matches: The transformation descriptions are semantically sound but don't match gold literal strings. This is expected for free-text fields and not necessarily a problem.
  • The agent adds an extra gravimetric_water_content category with null source values (mapping.json:74-82) — correctly documenting that this variable is not in the source data.

Manifest & Artifact Integrity

  • The heldout_harmonized_csv_manifest.json includes both raw and compressed SHA-256 hashes, which is good practice for tracking large files outside git.
  • The gzip compression achieves ~16× compression (116 MB → 7.2 MB), consistent with columnar CSV data.
  • No issues with the manifest format.

Security / Safety

  • No secrets, credentials, or sensitive data found.
  • The anti-leakage scan reported 0 matches — the agent did not access forbidden paths (gold output, other fold sandboxes).
  • The hardcoded paths (/h/jmc/, /scratch/jmc/) reference external data directories but don't expose any credentials.

Test Coverage

  • The PR includes a syntax check (python -m py_compile) and a forbidden-path rg scan, which are appropriate lightweight validations.
  • No unit tests for the harmonization logic itself, but this appears consistent with how other evaluation bundles are structured.
  • The 0/0 values in shared_row_value_matches mean no value-level testing occurred (no common key rows existed to compare). This is a gap worth noting in the evaluation, even if it's a consequence of the key policy.

Recommendations

  1. Investigate key mismatch root cause before merging, or add a note in EVALUATION.md documenting the suspected cause (e.g., site_id construction difference, timezone handling discrepancy).
  2. Remove the unused local_mst_to_utc_string function from harmonize_heldout.py:39-42.
  3. Document or parameterize the hardcoded paths in harmonize_heldout.py:9-11.
  4. Clarify the 2× row count discrepancy — either this is expected (gold applies additional filters not captured in the agent instructions) or it reveals a bug in the agent's exclusion logic.

The bundle is otherwise complete and well-documented. The artifact structure, manifest, and anti-leakage audit are sound.

@claude claude Bot mentioned this pull request Jun 30, 2026
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.

1 participant