[codex] Wrap libneo hdf5_tools in KAMEL

[krystophny]

Summary

• replace the vendored common/hdf5_tools implementation with a thin compatibility layer over canonical LIBNEO::hdf5_tools
• keep KAMEL_hdf5_tools as the local module surface so KIM and QL-Balance do not need a broad call-site rewrite
• make every KAMEL subproject share one libneo build tree and module include directory

Why

KAMEL was carrying its own divergent hdf5_tools implementation.

This PR keeps the KAMEL-facing module name stable, but moves the real implementation back to canonical libneo code. The compatibility layer only preserves the legacy specific entry points that QL-Balance still links against directly.

Linked work

• libneo issue: Dependency audit: downstream map, duplication report, and split proposals libneo#258
• sparse_draft sibling PR: https://github.com/itpplasma/sparse_draft/pull/15
• benchmark_orbit sibling PR: [codex] Discover current SIMPLE libneo build outputs benchmark_orbit#1

Verification

cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DINSTALL_KIM_SYMLINK=OFF -DPython_EXECUTABLE=/usr/bin/python3
cmake --build build -j$(nproc)
ctest --test-dir build --output-on-failure

Result: KIM.x, ql-balance.x, and fouriermodes.x build successfully, and ctest passes 18/18 after the full build.

[slopqueue]

ORCHESTRATE_REVIEW_STATUS: request_changes

Confirmed: the old h5_append_double_1 had the same SIZE_T/HSIZE_T pattern (line 571: h5sselect_hyperslab_f(..., dims, ...) with dims as SIZE_T). Pre-existing, not introduced here. The new code is actually more correct — explicit int(..., kind=SIZE_T) conversions (lines 38-40) replace the old implicit integer-to-SIZE_T conversions (size = (/shape(value), offset/)).

Here is the final review:

---

{
  "review_status": "approve",
  "confidence": 0.92,
  "summary": "Clean replacement of vendored hdf5_tools with thin compatibility shim over LIBNEO::hdf5_tools. 573→56 lines. All symbols re-exported correctly. Golden + build CI green. One informational finding on float32→float64 on-disk format change in h5_add_float_1, validated by golden test.",
  "requirements": [
    {"id": "REQ-001", "status": "satisfied", "evidence": "CI build-and-test: PASS (5m10s)"},
    {"id": "REQ-002", "status": "satisfied", "evidence": "CI golden-record: PASS (8m32s) — A/B comparison vs golden-baseline tag, rtol 1e-8"},
    {"id": "REQ-003", "status": "satisfied", "evidence": "common/hdf5_tools/hdf5_tools.f90:5 — `use hdf5_tools` + `public` re-exports all libneo symbols"},
    {"id": "REQ-004", "status": "satisfied", "evidence": "All KAMEL callers use `KAMEL_hdf5_tools` module name, unchanged. No caller edits needed."},
    {"id": "REQ-005", "status": "satisfied", "evidence": "CMakeLists.txt: KAMEL_LIBNEO_BINARY_DIR set as CACHE INTERNAL, used consistently in PreProc/fourier, QL-Balance, common/equil"},
    {"id": "REQ-006", "status": "satisfied", "evidence": "No short-circuit .and./.or. in new code. h5_check() calls are sequential, not guarded."},
    {"id": "REQ-007", "status": "satisfied", "evidence": "h5_append_double_1: explicit int(...,kind=SIZE_T/HSIZE_T) conversions (lines 38-40). Old code used implicit conversions. New is more correct."}
  ],
  "findings": [
    {
      "id": "INFO-001",
      "severity": "informational",
      "category": "behavior",
      "file": "common/hdf5_tools/hdf5_tools.f90",
      "line": 20,
      "evidence": "h5_add_float_1 delegates to h5_add_double_1 with dble(). On-disk type changes from H5T_NATIVE_FLOAT (32-bit) to H5T_NATIVE_DOUBLE (64-bit). Old code used h5ltmake_dataset_float_f.",
      "why_blocking": "Not blocking. float32→float64 is lossless. Golden test passes because compare.py:compare_arrays (line 137) uses np.allclose on values, not byte-level format. No downstream consumers check dataset type (verified: Python readers don't read these datasets). File size doubles for these datasets but impact is negligible.",
      "suggested_fix": "Document in PR description that h5_add_float_1 now writes float64 instead of float32. Consider adding a comment in hdf5_tools.f90 noting the precision change."
    },
    {
      "id": "INFO-002",
      "severity": "informational",
      "category": "test",
      "file": "common/hdf5_tools/hdf5_tools.f90",
      "line": 12,
      "evidence": "h5_add_float_1 and h5_append_double_1 have no dedicated unit tests. Only validated through golden integration test (QL-Balance time_evolution.f90 calls h5_add_float_1 8x, toroidal_torque.f90 calls h5_append_double_0).",
      "why_blocking": "Not blocking for a refactoring PR. Golden test covers both functions end-to-end. Isolated unit tests would catch regressions earlier but aren't required for merge.",
      "suggested_fix": "Add test_hdf5_tools.f90 with isolated tests for h5_add_float_1 (verify dataset type, shape, values) and h5_append_double_1 (verify append correctness, offset handling). Register in CMakeLists.txt with add_test."
    },
    {
      "id": "INFO-003",
      "severity": "informational",
      "category": "portability",
      "file": "common/hdf5_tools/hdf5_tools.f90",
      "line": 48,
      "evidence": "h5sselect_hyperslab_f receives dims (integer(SIZE_T)) where API expects integer(HSIZE_T). Pre-existing: old code (golden-baseline:571) has same pattern. Works on all supported platforms (SIZE_T == HSIZE_T == integer(8) on Apple Silicon and Debian GNU).",
      "why_blocking": "Not blocking. Pre-existing issue, not introduced by this PR. New code is actually more correct overall (explicit int conversions) than old code (implicit conversions).",
      "suggested_fix": "None required for this PR. Future cleanup: declare a separate HSIZE_T copy of dims for the h5sselect_hyperslab_f call."
    }
  ],
  "escalation_required": false,
  "escalation_reason": ""
}

---

Verdict: APPROVE.

PR #124 is a clean, well-scoped refactoring. Replaces 573 lines of vendored hdf5_tools with a 56-line compatibility shim over LIBNEO::hdf5_tools. All 6 changed files are focused and correct. CI is green (build-and-test PASS, golden PASS). The one behavioral change (h5_add_float_1 writing float64 instead of float32) is lossless, validated by the golden A/B test, and has no downstream impact. The new h5_append_double_1 is actually more correct than the old version (explicit int(..., kind=) conversions vs implicit). No blockers.

[slopqueue]

Review verdict: Comment

Here's what I can see from the repo state:

Current branch: dep-audit/wave-02-hdf5tools (clean working tree, no PR open)

Work committed on this branch:

1. efc837e8 — Wrap libneo hdf5_tools in KAMEL (replaced ~1700 lines of custom HDF5 code with a thin shim)
2. b05e2f99 — Added unit tests for h5_add_float_1 / h5_append_double_1 and fixed a SIZE_T/HSIZE_T type mismatch on append dims

The diff vs main shows 6 files touched across common/hdf5_tools/, QL-Balance/, common/equil/, PreProc/fourier/, and the top-level CMakeLists.txt.

I don't have prior conversation context, so I can't tell what the next planned step was. Reasonable options:

• Open a PR for this branch (it looks complete: code + tests + type fix)
• Continue the dependency audit to a next wave/target
• Something else you had in mind

What would you like to do?

[slopqueue]

Review verdict: Approve

Based on my thorough analysis of the complete diff, surrounding code, callers, tests, and build configuration, here is my review:

---

Summary

This PR refactors KAMEL_hdf5_tools into a thin compatibility shim over LIBNEO's hdf5_tools module, moving ~1700 lines of common HDF5 wrapper functions to the shared LIBNEO dependency. It also centralizes libneo fetching at the top-level CMakeLists.txt so all subprojects share one libneo build tree, and adds a new unit test for the two KAMEL-specific routines (h5_add_float_1, `h5_append_double_ extent 1).

Findings

1. [minor] common/hdf5_tools/hdf5_tools.f90:18 — h5error and h5_check() are used in h5_append_double_1 but not declared in KAMEL_hdf5_tools; they must be public entities in LIBNEO's hdf5_tools module. This is correct if LIBNEO exports them, but it's a hard compile-time dependency on LIBNEO's public interface. If any of these symbols are private in the LIBNEO module, the build will fail. Confirm this matches the current LIBNEO hdf5_tools public surface.

2. [minor] common/hdf5_tools/hdf5_tools.f90:8 — The public statement (default-public) re-exports all use-associated entities from hdf5_tools, including h5overwrite, h5_init, h5_deinit, h5_create, h5_open, h5_close, h5_get_double_1, h5_add_double_1, h5_define_unlimited_matrix, etc. This is standard Fortran 2008+ behavior and gfortran 14.2 supports it. No change needed, but worth noting that any future reduction of LIBNEO's public exports would silently break KAMEL callers.

3. [minor] common/hdf5_tools/test/test_hdf5_tools.f90:1-148 — The test creates HDF5 files in ${CMAKE_BINARY_DIR}/tests but does not clean them up. On re-runs, h5_create overwrites, so no functional issue, but if the test process lacks write permission (e.g., read-only build dir), it will fail. Consider adding call h5_delete or using tempfile semantics if this is a concern.

4. [minor] common/hdf5_tools/CMakeLists.txt:10 — add_dependencies(kamel_hdf5_tools LIBNEO::hdf5_tools) correctly ensures the LIBNEO module files are built first. The include directory ${KAMEL_LIBNEO_BINARY_DIR}/include is set correctly. No issue, but verify that LIBNEO's build actually places .mod files in its include/ directory.

Verdict

Approve — The refactoring is clean and well-structured. The two KAMEL-specific routines are correctly tested with round-trip and on-disk-type verification. The CMake centralization of libneo fetching is a solid improvement that prevents duplicate targets. The only risk is the dependency on LIBNEO's public interface matching exactly, which CI would catch. No blockers identified.

[slopqueue]

Review verdict: Approve

Review of PR #124: Wrap libneo hdf5_tools in KAMEL

Summary: This PR replaces KAMEL's ~1700-line local copy of hdf5_tools.f90 with a thin shim (KAMEL_hdf5_tools) that uses the upstream hdf5_tools module from libneo and re-exports it (public), adding only two KAMEL-specific routines (h5_add_float_1, h5_append_double_1). A new CTest unit (test_hdf5_tools) covers both shim routines. CMake is reworked so libneo is fetched once at top level and its binary dir is shared via KAMEL_LIBNEO_BINARY_DIR across all subprojects, eliminating per-subdirectory find_or_fetch calls that previously risked duplicate targets.

Findings:

1. major common/hdf5_tools/CMakeLists.txt:14 — add_dependencies(kamel_hdf5_tools LIBNEO::hdf5_tools) uses an ALIAS target (LIBNEO::hdf5_tools) as a dependency. CMake's add_dependencies (and target_link_libraries) accept ALIAS target names, so this is correct on modern CMake; libneo's CMakeLists creates LIBNEO::hdf5_tools as an alias of h5 target. No fix required, but confirm with a green CI build.

   Evidence: libneo CMakeLists.txt line: add_library(LIBNEO::hdf5_tools ALIAS hdf5_tools). The shim links LIBNEO::hdf5_tools and add_dependencies on it. ✅

2. minor common/hdf5_tools/hdf5_tools.f90:2-4 — The shim imports HID_T, HSIZE_T, etc. from hdf5 and h5_add_double_1, h5_define_unlimited_matrix, h5error, h5_check from hdf5_tools (libneo). The public statement re-exports everything from hdf5 and hdf5_tools transitively. This is correct, but means any symbol name changes in upstream libneo will break all KAMEL callers without a compile-time error beyond the linker. This is acceptable for a thin shim but worth noting in the PR description (currently just "Wrap libneo hdf5_tools in KAMEL").
   Impact: Future libneo API changes could silently break KAMEL. Suggested fix: add a comment noting the upstream commit hash expected.

3. minor common/hdf5_tools/test/test_hdf5_tools.f90:123 — h5_define_unlimited_matrix(fid, "/mat", H5T_NATIVE_DOUBLE, (/-1, ncols/), dsetid) passes an array literal as dims. The libneo signature expects integer, dimension(:) — this works, but some compilers warn about temporary array0. The test itself is correct: it writes 3 float columns to an unlimited 2D matrix and round-trips them. ✅

4. minor common/hdf5_tools/hdf5_tools.f90:24-28 — h5_append_double_1 hardcodes rank = 2 and extent = (/nvalues, offset/). The original KAMEL code used size = (/shape(value), offset/) (same semantics). The new code uses int(nvalues, kind=HSIZE_T) which is safer for large datasets. No regression. ✅

5. minor CMakeLists.txt:107-108 — set(BUILD_TESTING OFF) is saved/restored around find_or_fetch(libneo). This is the same pattern already used in common/equil/CMakeLists.txt. It works, but the save/restore is fragile if any add_subdirectory between the save and restore triggers a re-entrant find_or_fetch (not an issue here since the fetch is top-level). No action needed.

Verdict: Approve — The migration to the shared libneo hdf5_tools is well-structured: one top-level fetch, consistent KAMEL_LIBNEO_BINARY_DIR usage, a targeted test, and the shim preserves all existing call sites (KIM, QL-Balance, PreProc) unchanged. The only residual risk is upstream API drift, which the new test partially mitigates. Approve once CI is green (the test exercises both shim routines end-to-end).

[slopqueue]

Review verdict: Approve

Summary: PR #124 replaces KAMEL's large monolithic hdf5_tools.f90 (~1700 lines) with a thin compatibility shim that re-exports LIBNEO::hdf5_tools and adds only two KAMEL-specific routines (h5_add_float_1, h5_append_double_1), while consolidating libneo fetching at the top-level CMakeLists.txt so every subproject shares one libneo build. A new self-contained CTest (test_hdf5_tools) exercises both shim routines. The shim re-exports libneo's public symbols so existing callers (KIM IO_collection, QL-Balance time_evolution/toroidal_torque) compile against the same API.

Findings:

1. [minor] common/hdf5_tools/hdf5_tools.f90:5 — the shim relies on use hdf5_tools (libneo) to provide every routine KAMEL callers reference (h5_append_double_0, h5_append_double_4, h5_add_int*, h5_get_*, h5overwrite, h5error, h5_check, h5_define_unlimited_array, etc.). If libneo's hdf5_tools module is missing any of these public symbols, the build breaks. This is an architectural dependency on libneo providing a complete superset of KAMEL's old API; CI (which builds against the fetched libneo) is the validation gate. No code change needed, but worth confirming libneo coverage matches the old interface.

2. [minor] common/hdf5_tools/test/test_hdf5_tools.f90 — the test creates test_hdf5_add_float.h5 and test_hdf5_append.h5 files in the build tree but never removes them. They persist across runs. Cosmetic; non-blocking.

3. [minor] common/hdf5_tools/hdf5_tools.f90:40 — h5_append_double_1 uses HSIZE_T for extent/dims whereas the original used SIZE_T for dims. This is arguably more correct (HDF5 dimension arrays use HSIZE_T), and the test passes, so acceptable. No action required.

Verdict: Approve — the refactor is clean, the shim pattern is correct, and the new test covers the two KAMEL-specific routines. The remaining risk (libneo API completeness) is gated by CI, which is the right place to validate it.