Refactor expert harmonizer from monolith to modular package

[cmungall]

Summary

Refactored the expert soil-moisture harmonization script from a single 1200-line monolith into a modular package structure. This enables cleaner leave-one-cluster-out evaluation without text-splicing, as held-out datasets are simply omitted from the module registry rather than surgically removed from source code.

Key Changes

• Removed monolith: Deleted data/gold/expert_code/harmonize_ess-dive_soilmoisture_data.py (1239 lines)
• Created modular package data/gold/expert_code/harmonize_sm/:
  • common.py: Shared library with Context class (wraps mapping JSON + dataset-access helpers), pure transformation helpers, location deduplication, and CSV output writer
  • dataset_NN.py (19 modules): One per dataset (indices 1-10, 15-18, 23-27), each exposing harmonize(ctx) -> DatasetResult
  • datasets.py: Registry mapping dataset indices to harmonizer functions
  • run.py: Runner script that loads datasets and optionally excludes a holdout set
  • README.md: Package documentation
• Updated fold evaluation:
  • Replaced src/folds/ablate_monolith.py (cell-splicing logic) with src/folds/expert_harmonizer.py (modular runner)
  • New approach: kept_module_paths() returns code context (common + kept datasets), run() executes without held-out modules
  • No coupling to cv_folds.yaml; validation automatically rejects reference dataset (0) and globally-excluded datasets (11-14, 19-22)
• Updated tests: Replaced tests/test_ablate_monolith.py with tests/test_expert_harmonizer.py
• Updated documentation: Modified skill READMEs to reference the new modular structure

Implementation Details

• Backward compatibility: Harmonized outputs are identical to the monolith (helper bodies are verbatim copies)
• Wiring changes only: Globals → Context instance, accumulator lists → DatasetResult return values
• No cross-dataset dependencies: Only the reference dataset (index 0) is shared; holding out any other dataset leaves the rest runnable
• Cleaner LOO: Holdout specification is now --holdout 1,2,3,6,16,27 (dataset indices or identifiers) rather than text-based cell removal

https://claude.ai/code/session_01YDyuNoMsjJWqNRsdHLHrSh

[claude]

Claude finished @cmungall's task —— View job

---

PR Review: Refactor expert harmonizer from monolith to modular package

Tasks

• Gather context and understand the request
• Read key source files (common.py, run.py, datasets.py, expert_harmonizer.py)
• Read sample dataset modules and tests
• Provide comprehensive review feedback

---

This is a well-structured refactor. The core idea—replacing text-splicing with module-omission for LOO evaluation—is sound and the implementation is clean. Here are my findings:

Architecture & Code Quality

Strengths:

• Clean separation of concerns: common.py for shared utilities, dataset_NN.py for per-dataset logic, datasets.py as a registry, run.py as the entry point.
• DatasetResult as a return value instead of global accumulators is a significant improvement for testability and clarity.
• Context dataclass cleanly encapsulates what were previously scattered globals.
• resolve_holdout supporting both integer indices and string dataset_identifiers is a nice UX improvement.

Issues / Suggestions:

1. Unused variable in every dataset module (src/folds/expert_harmonizer.py-level issue extends to all dataset_NN.py)

In every harmonize() function, REF_IDX is extracted from ctx but most dataset files don't use it (e.g., dataset_01.py:26, dataset_08.py:26, dataset_16.py:26). Only datasets that actually read from the reference dataset (like dataset_23.py:26, dataset_27.py:26) need it. This creates dead code in 15+ files and could mislead maintainers.

2. O(n²) pairwise comparison in harmonize_locations (common.py:270)

for i, j in combinations(range(n), 2):
    if is_match_pair(i, j, loc_df):

With n location rows this is O(n²). The docstring says it's a "verbatim port" from the monolith, so this is inherited, but worth noting. For large datasets this could be slow. A spatial index (KD-tree or blocking by grid cell) would scale better.

3. run.py mutates sys.path at module level (run.py:22)

sys.path.insert(0, str(Path(__file__).resolve().parent))

This is a side effect of import and will persist for the lifetime of the process. If run.py is imported (rather than executed directly), it silently modifies sys.path. The same pattern appears in expert_harmonizer.py:147. Better to do this conditionally or use a proper package __init__.py.

4. datasets.py imports all modules eagerly at import time (datasets.py:20)

DATASETS = {idx: import_module(f"dataset_{idx:02d}").harmonize for idx in DATASET_INDICES}

All 19 dataset modules are imported when datasets.py is imported, even if only a subset are needed. This means importing datasets always requires all dependencies (numpy, pandas, pyproj, etc.) to be available. Fine for the production use-case, but adds unnecessary overhead for tools that only need the registry (e.g., expert_harmonizer.py which imports from datasets.py just for DATASET_INDICES).

5. DATASET_INDICES is duplicated (expert_harmonizer.py:33, datasets.py:18)

Both files maintain the same list independently. The comment in expert_harmonizer.py says "Kept in sync with harmonize_sm/datasets.py", which is a maintenance hazard. expert_harmonizer.py could import this from the package instead of duplicating it (though the lazy-import design in run() is intentional to avoid importing the scientific stack — a comment explaining this tradeoff would help).

6. read_ds_csv silently ignores errors kwarg (common.py:90-93)

def read_ds_csv(self, idx: int, filename: str, encoding="utf-8", errors="ignore", **kwargs) -> pd.DataFrame:
    return pd.read_csv(self.ds_path(idx) / filename, encoding=encoding, **kwargs)

The errors parameter is in the signature but not passed to pd.read_csv. This is a bug: callers expecting encoding errors to be ignored will instead get an exception (pandas' default is errors="strict"). Should be pd.read_csv(..., encoding=encoding, encoding_errors=errors, **kwargs) (pandas ≥ 1.3) or kept as a doc-only note if intentional.

7. UUID stability not guaranteed (common.py:288-290)

uuid_map = pd.DataFrame({
    'location_component_id': comp_ids,
    'harmonized_location_uuid': [str(uuid.uuid4()) for _ in comp_ids]
})

UUIDs are random on every run, so the same physical location gets a different UUID each time harmonize_locations is called. This may be intentional (the docstring says "the written CSV is the raw concat"), but if anyone uses harmonized_location_uuid for cross-run joins or comparison, they'll get false negatives. A deterministic UUID (e.g., uuid.uuid5(NAMESPACE, canonical_site_name)) would be more useful.

Potential Bugs

8. _validate_holdout called twice in expert_harmonizer.run() (expert_harmonizer.py:144, then delegated to harmonize_sm/run.py which calls it again via the holdout check at line 43-48)

This is harmless but redundant. The outer validation in expert_harmonizer.py:144 runs against the on-disk package, which is correct; the inner check in run.py:43-48 is against DATASET_INDICES. They should always agree, but if they somehow diverge (e.g., a module file is present but not listed) the error messages will differ.

9. assemble_source reads files with no encoding specification (expert_harmonizer.py:92)

chunks.append(f"# ===== {path.name} =====\n{path.read_text()}")

Path.read_text() uses the system locale encoding. On Windows or non-UTF-8 locales this could fail or produce garbled output for any non-ASCII content in the source files. Use path.read_text(encoding="utf-8") for portability.

Test Coverage

The tests are solid for the pure helpers (resolve_holdout) and the structural properties (module lists, path inclusion/exclusion, Python parseability). Two gaps worth considering:

• No integration test that the runner produces identical outputs to the monolith. The PR claims backward compatibility, but there's no assertion verifying this (likely intentional given the test environment doesn't have the raw datasets, but worth a note in the test file).
• test_assemble_source_excludes_holdout_code_keeps_rest (test_expert_harmonizer.py:89-95) checks for "idx = 27" not in the output and "idx = 5" in it. This couples the test to an internal variable name inside the dataset modules. If a module is reorganized (e.g., the idx = N line is removed), the test would silently pass even if the wrong module was included. Checking for f"dataset_{i:02d}.py" header lines (which assemble_source adds) would be more robust.

Security

No security concerns. The code reads local CSV files and JSON; it doesn't execute user-supplied strings, make network requests, or expose any attack surface.

Performance

Beyond the O(n²) location-matching noted above, the modular structure is actually a performance improvement for LOO: loading only the needed modules avoids processing excluded datasets entirely.

---

Summary: This is a clean and well-motivated refactor. The main actionable bug is the dropped errors kwarg in read_ds_csv (#6). The DATASET_INDICES duplication (#5) and the sys.path mutation side effect (#3) are the most significant maintainability concerns. Everything else is minor or inherited from the monolith.