Skip to content

[codex] add fold01 agent evaluation bundle#37

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

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

Conversation

@jmchandonia

Copy link
Copy Markdown
Collaborator

Summary

  • add tracked fold-01 clustered holdout artifacts for indices 1, 2, 3, 6, 16, and 27
  • add action trace, generated outputs, evaluation report, and machine-readable metrics
  • track the combined heldout_harmonized.csv as heldout_harmonized.csv.gz because the raw combined CSV exceeds GitHub's 100 MB file limit

Evaluation Highlights

  • targeted forbidden-path scan found zero matches
  • agent produced 3,584,771 rows vs 2,214,708 gold rows
  • dataset-index-aware key precision 0.364 and gold recall 0.621
  • shared-row matches: interval 1,306,902/1,307,069; VWC 1,238,017/1,307,069; water potential 1,306,739/1,307,069
  • cluster mapping literal comparison was not computed by the helper

Validation

  • python -m py_compile evaluations/fold-01-holdout-1-2-3-6-16-27/agent_outputs/harmonize_heldout.py
  • targeted rg forbidden-path scan over evaluations/fold-01-holdout-1-2-3-6-16-27/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: fold01 agent evaluation bundle

Tasks:

  • Analyzing PR context
  • Reading repository guidelines (CLAUDE.md) — no CLAUDE.md found in repo
  • Reviewing key files
  • Providing feedback

Summary

This PR adds a complete agent evaluation bundle for fold-01, covering held-out dataset indices 1, 2, 3, 6, 16, and 27. The evaluation infrastructure is well-organized, with a clear separation between agent outputs, action logs, and evaluation metrics. Below is detailed feedback.


Code Quality & Best Practices

harmonize_heldout.py

Overall the script is clean and functional, but has several issues worth addressing:

  1. Hardcoded absolute paths (harmonize_heldout.py:8-11): FOLD and RAW use machine-specific absolute paths (/scratch/jmc/..., /h/jmc/...). These paths are non-reproducible and will fail on any system other than the original author's machine. Consider using environment variables or a config file:

    FOLD = Path(os.environ.get('FOLD_SANDBOX', '/scratch/jmc/data-harmonization-eval/.runs/fold-01-holdout-1-2-3-6-16-27'))
    RAW = Path(os.environ.get('RAW_DATA', '/h/jmc/ess-dive_wfsfa_soil_datasets'))
  2. Logging timestamps are static (harmonize_heldout.py:34): The log() function writes a hardcoded 2026-06-30T00:00:00-07:00 timestamp rather than datetime.utcnow().isoformat(). This means the action log entries generated by the script cannot be used to reconstruct execution timing.

  3. groupby with dropna=False aggregation (harmonize_heldout.py:84): The finalize_long() function uses groupby(..., dropna=False).agg({'volumetric_water_content_m3_m3': 'first', ...}). Using 'first' aggregation silently drops duplicate measurements for the same key; if a dataset has multiple sensors at the same key position, only the first will be kept. This should be documented or use a more intentional aggregation (e.g., mean, or flagging conflicts).

  4. add_intervals called after finalize_long merge (harmonize_heldout.py:89): Intervals are computed on the already-merged DataFrame, but the merge itself uses groupby that does not preserve sort order (though add_intervals does re-sort). This is correct but subtle; a comment would help maintainers.

  5. ds16() uses itertuples with attribute access (harmonize_heldout.py:155): meta.itertuples() on column names with hyphens (e.g. SITE_ID, VARIABLE) could fail if column names have special characters. The code appears safe here, but it's fragile. A dictionary-based approach (meta.set_index(...).to_dict()) is more robust.

  6. No error handling for missing files: Functions like ds1(), ds2(), etc. will raise uncaught FileNotFoundError if any expected input file is missing. This makes debugging harder. Consider wrapping reads in try/except with informative messages.

  7. ds27() cross-dataset depth inference (mapping_notes.md:17): The note documents that S1–S4 depths for dataset 27 are inferred from dataset 16 metadata. This is a methodological choice that could introduce bias. It's properly documented in mapping_notes.md, but would benefit from an inline comment in the code at harmonize_heldout.py:177-181.


Potential Bugs

  1. Row count off-by-one discrepancy: verification_summary.json records dataset 1 as 1,373,556 rows, but the ADDED CSV (heldout_dataset_01_harmonized.csv) shows 1,373,557 lines (including header). This is normal (header + data), but worth confirming the manifest math is using row-count (not line-count) consistently.

  2. interval_min computed from merged rows (harmonize_heldout.py:72-75): For datasets with both VWC and water-potential sensors at the same depth, finalize_long merges them before computing intervals. If the two variable types have different sampling frequencies, the merged interval will reflect the combined timeline rather than the individual sensor timeline. This may explain some of the 167 non-matching interval_min rows reported in the evaluation.

  3. Dataset 6 site column handling (harmonize_heldout.py:146): df['site'] is passed directly as site_id. If this column has NaN values or inconsistent formatting, it could silently propagate into the output. A .fillna('unknown').str.strip() guard would be safer.


Performance Considerations

  1. finalize_long concatenates many small DataFrames: For dataset 1 with 9 files and many columns, the list of small DataFrames can be large. pd.concat is called once at the end which is efficient, but intermediate rows.append() inside column loops could create hundreds of tiny frames. This is not a correctness issue but may be slow for large datasets.

  2. ds16() uses depth_map.get((site, col), np.nan) in a loop: For each file and each column, a dictionary lookup is performed. This is fine at the current scale, but would not scale to thousands of columns.


Security Concerns

  1. No security concerns identified: The script reads from local filesystems using hardcoded package identifiers. There is no network access, no subprocess calls, no eval/exec, and no user-supplied input. The anti-leakage audit (0 forbidden-path matches) was properly conducted.

  2. Anti-leakage audit is adequate: The forbidden-path scan described in EVALUATION.md and verification_summary.json provides appropriate assurance that the agent did not access gold data.


Evaluation Completeness

  1. JSON mapping comparison was not computed (evaluation_metrics.json:43-50): The json_mapping_equivalence section records "note": "cluster mapping literal comparison not computed". This is a gap in the evaluation — the agent's mapping.json was not compared against the gold mapping for these held-out indices. This should be addressed in a future evaluation pass.

  2. Precision/recall interpretation: The precision of 0.364 and recall of 0.621 should be interpreted in the context that key_includes_dataset_index=True — the keying policy means rows from different datasets can't collide. The agent produced 62% of gold-matched rows, and produced 2.28M extra rows not in gold. Understanding whether these extra rows represent legitimate additional data or harmonization errors would improve the evaluation's informativeness.

  3. gravimetric_water_content_gH2O_gs is always NaN: The matched rows show 1,307,069/1,307,069 matches for gravimetric_water_content_gH2O_gs, which is perfect — but this is almost certainly because both agent and gold outputs have all-NaN for this column (none of the held-out datasets appear to include gravimetric measurements). The evaluation metric is technically correct but potentially misleading.


Minor Issues

  • AGENT_ACTION_LOG.md timestamps: entries 1-2 use 2026-06-30T00:00:00-07:00 (placeholder), then entries from line 13 onward use real timestamps starting 2026-06-30T15:54:21-0700. This inconsistency in the log is cosmetic but worth noting.
  • heldout_harmonized_csv_manifest.json:9 references a local_raw_path that is machine-specific and will not be meaningful to external evaluators.

Overall Assessment

The PR is well-structured and the evaluation methodology is sound. The primary issues are:

  • Non-reproducible paths in the harmonization script (high priority for reusability)
  • Static timestamps in the log function (low priority but misleading)
  • Missing JSON mapping comparison (evaluation gap)
  • Several minor defensive coding improvements recommended

The data outputs appear consistent with the reported metrics, and the anti-leakage audit provides reasonable confidence in the evaluation's integrity.

@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