Combined: validation suite + DNS/LES/IBM infrastructure

[sbryngelson]

Summary

Combined PR merging two feature branches:

From PR #35 (original)

• FFT Poisson solver support for stretched-y grids (22-32x speedup)
• Volume-weighted mean subtraction for FFT solvability on non-uniform grids
• RANS transport model stability fixes (point-implicit destruction terms)
• GPU residual NaN-masking fix
• Test suite refactoring (-748 LOC), test utility 2D k-indexing fix
• Thomas solver MAX_NY=4096 safety limit
• FFT2D nullptr guard for uniform grids

From PR #36 (merged in)

• MPI z-slab decomposition + halo exchange (CPU + GPU-direct)
• 5 LES SGS models (Smagorinsky, WALE, Vreman, Sigma, Dynamic Smagorinsky) — all GPU-accelerated
• IBM direct forcing with cylinder/sphere/NACA SDFs — fully GPU-accelerated with pre-computed weight arrays
• CMakePresets.json (8 build presets)
• New app entry points (cylinder, airfoil)
• Comprehensive documentation overhaul

Test plan

• CPU CI (Ubuntu + macOS, Debug + Release) passes
• GPU CI passes
• Existing tests unaffected by merge

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a turbulence validation suite: new GPU-marked CMake tests, multiple C++ validation tests (Poiseuille, DNS channel, RANS channel, TGV, cross-geometry), embedded reference data, reporting scripts, a reference-data downloader, and SLURM orchestration for full-scale validation.

Changes

Cohort / File(s)	Summary
CMake test registrations CMakeLists.txt	Register new medium/GPU validation tests and set OMP_TARGET_OFFLOAD=MANDATORY and timeouts for GPU-marked tests.
Cross-geometry smoke & multi-check tests tests/test_turbulence_cross_geometry.cpp	New comprehensive smoke and consistency test with multiple sections covering channel/duct/TGV geometries, realizability, EARSM checks, ordering, and profile-shape analyses.
Poiseuille validation tests/test_poiseuille_validation.cpp	Laminar Poiseuille validation suite: error metrics, incompressibility checks, multi-grid runs and grid-convergence assessment.
RANS channel validation tests/test_rans_channel_validation.cpp	Multi-model RANS channel harness exercising algebraic, transport, EARSM, and NN models; per-model diagnostics and law-of-wall checks.
DNS channel validation tests/test_dns_channel_validation.cpp	GPU-only DNS channel validation harness (stretched y), long-run energy diagnostics, filtering/trip forcing, and stability/incompressibility checks.
TGV validation tests/test_tgv_validation.cpp	Taylor–Green vortex tests (Re=100, Re=1600) with energy decay, dissipation proxy, divergence and symmetry diagnostics.
Reference data header tests/reference_data.hpp	Adds nncfd::reference embedded MKM DNS profiles, Reynolds stresses, Brachet TGV constants, law-of-wall helper, and Poiseuille analytic utilities (inline/constexpr).
Report & orchestration scripts scripts/generate_validation_report.py, scripts/run_validation.sh, scripts/download_reference_data.sh	Adds Python report generator (plots, L2/Linf metrics, error summary), SLURM orchestration script for Tier‑2 validation runs, and downloader/creator for MKM/Brachet reference data.
Docs / plans docs/plans/...2026-03-01-*.md	Adds design and implementation plan documents describing validation tiers, test configs, reference assets, orchestration, and reporting workflow.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Dev as Developer/CI
  participant CI as CI Runner
  participant Data as Reference Data
  participant GPU as GPU Node/Binary
  participant SLURM as SLURM Scheduler
  participant Report as Report Generator

  Dev->>CI: push PR (tests + scripts + data)
  CI->>Data: ensure reference data (download_reference_data.sh)
  CI->>GPU: run GPU-marked CMake tests
  CI-->>Dev: report CI results

  Dev->>SLURM: run_validation.sh (submit job)
  SLURM->>GPU: start long validation jobs (DNS, RANS, Poiseuille, TGV)
  GPU->>Data: read reference data
  GPU-->>SLURM: produce logs/artifacts
  SLURM->>Report: invoke generate_validation_report.py
  Report-->>Dev: plots + error_summary.txt

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add comprehensive CI test suite for DNS/RANS validation #31 — Overlapping additions of validation tests, CMake GPU registrations, and reference/test assets.
• Refactor test infrastructure and add shared utility headers #29 — Introduces test-harness and CMake helpers used by the new registrations and tests.
• Fix CPU/GPU consistency tests and rename ci script #18 — Related GPU/offload CI handling and OMP_TARGET_OFFLOAD consistency changes.

Suggested labels

Review effort 4/5

Poem

I’m a rabbit in a lab of flow,
Hopping through meshes row by row,
Channels, TGVs and Poiseuille too,
Scripts and data stitched in blue,
Validation hops — a joyful view! 🐇

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title is generic and does not clearly summarize the primary change; it uses vague terms like 'Combined' and 'infrastructure' that lack specificity about the main deliverable.	Revise to be more specific about the main change, e.g., 'Add turbulence validation test suite with four tier-1 tests' or 'Introduce DNS, RANS, and TGV validation tests with reference data'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch checking

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Swallowed exception: run_model_metrics() uses a blanket catch (...) that silently converts failures into ok=false without capturing or reporting actionable context about what failed. Referred Code } catch (...) { m.ok = false; } Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Exception details printed: Multiple test paths print raw exception messages via std::cerr << e.what() which could expose internal implementation details depending on what underlying libraries include in what(). Referred Code result.message = std::string("Exception: ") + e.what(); } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Tests reveal a critical CPU/GPU divergence The CPU and GPU implementations for 3D turbulence models are divergent, with the CPU path being only partially 3D. This core architectural flaw should be fixed instead of adding tests that work around it. Examples: tests/test_turbulence_cross_geometry.cpp [500] /// Uses 2D mesh because turbulence model CPU paths are 2D-only. tests/test_turbulence_cross_geometry.cpp [591] // Use 2D mesh — turbulence model CPU paths are 2D-only Solution Walkthrough: Before: // test_turbulence_cross_geometry.cpp // 3D smoke tests run on 3D meshes, implicitly testing the GPU path. static void test_3d_channel_smoke() { Mesh mesh; mesh.init_uniform(16, 16, 16, ...); // 3D mesh run_3d_smoke(type, mesh, ...); } // Physics validation tests run on 2D meshes due to CPU limitations. /// Uses 2D mesh because turbulence model CPU paths are 2D-only. static ModelMetrics run_model_metrics(...) { Mesh mesh; mesh.init_uniform(16, 32, ...); // 2D mesh // ... run solver on 2D mesh } static void test_model_ordering() { // This test is forced to use 2D metrics. auto laminar = run_model_metrics(TurbulenceModelType::None); ... } After: // In turbulence model implementation (e.g., turbulence_earsm.cpp) // The CPU implementation should be updated to be fully 3D. class SSTWithEARSM { void update(..., const Mesh& mesh, ...) { // ... // This loop should be 3D for CPU path, not 2D. FOR_INTERIOR_3D(mesh, i, j, k) { // Full 3D computation of turbulence quantities. } } }; // In test_turbulence_cross_geometry.cpp // Physics validation tests can now run on 3D meshes. static ModelMetrics run_model_metrics_3d(...) { Mesh mesh; mesh.init_uniform(16, 32, 16, ...); // 3D mesh // ... run solver on 3D mesh } static void test_model_ordering() { // Test can now use 3D metrics for consistent CPU/GPU validation. auto laminar = run_model_metrics_3d(...); ... } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical architectural flaw where CPU and GPU turbulence model implementations diverge for 3D cases, which the new tests work around instead of addressing.	High
General	Add missing check for SST model In test_model_ordering, add an assertion to verify that sst.u_max is less than laminar.u_max, consistent with the existing check for the baseline model. tests/test_turbulence_cross_geometry.cpp [559-584] static void test_model_ordering() { auto laminar = run_model_metrics(TurbulenceModelType::None); auto baseline = run_model_metrics(TurbulenceModelType::Baseline); auto sst = run_model_metrics(TurbulenceModelType::SSTKOmega); record("Ordering: all runs succeeded", laminar.ok && baseline.ok && sst.ok); if (!laminar.ok || !baseline.ok || !sst.ok) return; // Turbulence adds diffusion → flattens profile → lower u_max record("Ordering: Baseline u_max < Laminar u_max", baseline.u_max < laminar.u_max); + record("Ordering: SST u_max < Laminar u_max", + sst.u_max < laminar.u_max); std::cerr << " Laminar u_max=" << laminar.u_max << ", Baseline u_max=" << baseline.u_max << ", SST u_max=" << sst.u_max << "\n"; // Turbulent models should have nonzero eddy viscosity record("Ordering: Laminar nu_t == 0", laminar.mean_nu_t < 1e-15); record("Ordering: Baseline nu_t > 0", baseline.mean_nu_t > 1e-10); record("Ordering: SST nu_t > 0", sst.mean_nu_t > 1e-10); std::cerr << " Laminar nu_t=" << laminar.mean_nu_t << ", Baseline nu_t=" << baseline.mean_nu_t << ", SST nu_t=" << sst.mean_nu_t << "\n"; } Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a missing assertion in the test_model_ordering test, improving its completeness by verifying that the SST model also results in a lower max velocity than the Laminar model.	Low
	Add missing energy decay check In test_tgv_energy_ordering, add an assertion to verify that E_sst is less than or equal to E_laminar, consistent with the existing check for the Baseline model. tests/test_turbulence_cross_geometry.cpp [690-742] static void test_tgv_energy_ordering() { double L = 2.0 * M_PI; Mesh mesh; mesh.init_uniform(16, 16, 16, 0.0, L, 0.0, L, 0.0, L); auto run_tgv = [&](TurbulenceModelType type) -> double { Config config; config.nu = 0.01; config.dt = 0.005; config.turb_model = type; config.verbose = false; RANSSolver solver(mesh, config); solver.set_velocity_bc(create_velocity_bc(BCPattern::FullyPeriodic)); if (type != TurbulenceModelType::None) solver.set_turbulence_model(create_turbulence_model(type)); // TGV IC for (int k = mesh.k_begin(); k < mesh.k_end(); ++k) for (int j = mesh.j_begin(); j < mesh.j_end(); ++j) for (int i = mesh.i_begin(); i <= mesh.i_end(); ++i) solver.velocity().u(i, j, k) = std::sin(mesh.xf[i]) * std::cos(mesh.y(j)) * std::cos(mesh.z(k)); for (int k = mesh.k_begin(); k < mesh.k_end(); ++k) for (int j = mesh.j_begin(); j <= mesh.j_end(); ++j) for (int i = mesh.i_begin(); i < mesh.i_end(); ++i) solver.velocity().v(i, j, k) = -std::cos(mesh.x(i)) * std::sin(mesh.yf[j]) * std::cos(mesh.z(k)); solver.sync_to_gpu(); for (int step = 0; step < 100; ++step) solver.step(); solver.sync_from_gpu(); return solver.compute_kinetic_energy(); }; double E_laminar = run_tgv(TurbulenceModelType::None); double E_baseline = run_tgv(TurbulenceModelType::Baseline); double E_sst = run_tgv(TurbulenceModelType::SSTKOmega); std::cerr << " TGV E_final: Laminar=" << E_laminar << ", Baseline=" << E_baseline << ", SST=" << E_sst << "\n"; // More effective viscosity → faster energy decay → lower final energy // Baseline adds Smagorinsky-like nu_t → should decay faster than laminar record("TGV ordering: E_baseline <= E_laminar", E_baseline <= E_laminar * 1.01); // 1% tolerance + record("TGV ordering: E_sst <= E_laminar", + E_sst <= E_laminar * 1.01); // 1% tolerance // All energies should be positive and finite record("TGV ordering: all energies valid", std::isfinite(E_laminar) && std::isfinite(E_baseline) && std::isfinite(E_sst) && E_laminar > 0 && E_baseline > 0 && E_sst > 0); } Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a missing assertion in the test_tgv_energy_ordering test, improving its completeness by verifying that the SST model's final energy is also less than the Laminar model's.	Low
Update

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_turbulence_cross_geometry.cpp (1)

695-723: Consider adding initialize_uniform() for transport model consistency.

Similar to the issue in test_cross_geometry_consistency, the run_tgv lambda doesn't call initialize_uniform() for transport models like SSTKOmega. While the test currently only validates that energies are finite and positive, uninitialized k/omega could lead to unexpected behavior or silent numerical issues.

♻️ Optional improvement

         RANSSolver solver(mesh, config);
         solver.set_velocity_bc(create_velocity_bc(BCPattern::FullyPeriodic));
         if (type != TurbulenceModelType::None)
             solver.set_turbulence_model(create_turbulence_model(type));
+        solver.initialize_uniform(1.0, 0.0);

         // TGV IC

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_turbulence_cross_geometry.cpp` around lines 695 - 723, The run_tgv
lambda currently sets a turbulence model for types != TurbulenceModelType::None
but never initializes transport fields; call RANSSolver::initialize_uniform()
(or solver.initialize_uniform()) right after
solver.set_turbulence_model(create_turbulence_model(type)) and before
solver.sync_to_gpu() so SSTKOmega or other transport models get initialized
(ensure this occurs only when type != TurbulenceModelType::None).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_turbulence_cross_geometry.cpp`:
- Around line 444-471: The SST k-omega transport model fields aren’t being
initialized here; after creating the turbulence model
(solver.set_turbulence_model(create_turbulence_model(type))) and before stepping
or sync_to_gpu(), call the solver.initialize_uniform(...) used elsewhere (e.g.
solver.initialize_uniform(1.0, 0.0)) when the chosen model is SSTKOmega (or
whenever the transport model requires k/omega initialization) so k and omega are
set and nu_t computations won’t divide by zero or use uninitialized values.

---

Nitpick comments:
In `@tests/test_turbulence_cross_geometry.cpp`:
- Around line 695-723: The run_tgv lambda currently sets a turbulence model for
types != TurbulenceModelType::None but never initializes transport fields; call
RANSSolver::initialize_uniform() (or solver.initialize_uniform()) right after
solver.set_turbulence_model(create_turbulence_model(type)) and before
solver.sync_to_gpu() so SSTKOmega or other transport models get initialized
(ensure this occurs only when type != TurbulenceModelType::None).

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c6bb3e and 0537872.

📒 Files selected for processing (2)

• CMakeLists.txt
• tests/test_turbulence_cross_geometry.cpp

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

tests/test_turbulence_cross_geometry.cpp (3)

829-844: Run the local GPU pre-CI suite before merge

Given this file explicitly addresses latent GPU initialization behavior, it’s worth running the GPU preflight script prior to merge.

Based on learnings: For GPU-related changes, also run ./test_before_ci_gpu.sh to validate GPU CI test suite locally including physics validation tests, turbulence model validation, and CPU/GPU consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_turbulence_cross_geometry.cpp` around lines 829 - 844, The test
suite touches GPU-initialization-sensitive cases (see main() and the listed
tests like test_3d_channel_smoke, test_3d_duct_smoke, test_3d_tgv_smoke,
test_dns_combo, test_cross_geometry_consistency), so before merging run the GPU
pre-CI locally: execute ./test_before_ci_gpu.sh with the correct
CUDA_VISIBLE_DEVICES and environment modules loaded, confirm the script
completes and that GPU vs CPU consistency and turbulence model GPU tests (the
ones invoked by run_sections) pass, and fix any GPU initialization failures
reported before pushing the merge.

---

219-230: Extract a shared TGV initializer to avoid drift

The TGV velocity initialization is duplicated in two sections. A small helper would reduce maintenance risk if one path changes later.

Also applies to: 726-735

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_turbulence_cross_geometry.cpp` around lines 219 - 230, Extract the
duplicated Taylor–Green vortex initialization into a shared helper (e.g.,
initializeTaylorGreenVortex or initTGV) that accepts the mesh and a reference to
the solver.velocity() (or the velocity field object) and performs the three
nested loops that assign u and v using the same formulas (u: sin(mesh.xf[i]) *
cos(mesh.y(j)) * cos(mesh.z(k)); v: -cos(mesh.x(i)) * sin(mesh.yf[j]) *
cos(mesh.z(k))). Replace both duplicated blocks with calls to this helper so
both locations (including the other occurrence at lines 726-735) use the same
function and remove duplicated loop code.

---

751-759: Section intent and asserted ordering are currently misaligned

This section is titled as an ordering check, but it only asserts Baseline <= Laminar. Consider either adding an SST ordering assertion or renaming the check/section to reflect baseline-vs-laminar only.

💡 Optional assertion to match section intent

         record("TGV ordering: E_baseline <= E_laminar",
                E_baseline <= E_laminar * 1.01);  // 1% tolerance
+        record("TGV ordering: E_sst <= E_baseline",
+               E_sst <= E_baseline * 1.05);  // keep tolerance slightly looser if needed

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_turbulence_cross_geometry.cpp` around lines 751 - 759, The test's
"ordering" section only asserts E_baseline <= E_laminar but mentions ordering
for all schemes; update the check to either (A) add an SST ordering assertion
using the same pattern (e.g., add a record call that asserts E_baseline <= E_sst
&& E_sst <= E_laminar or two comparisons with the same ~1% tolerance)
referencing the existing record function and variables E_baseline, E_sst,
E_laminar, or (B) if you only mean to compare baseline vs laminar, rename the
existing record description to something like "TGV: E_baseline <= E_laminar" to
reflect the narrower intent. Ensure you keep the finite/positive energy check
as-is.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_turbulence_cross_geometry.cpp`:
- Around line 295-306: The test initializes only the u-component and leaves v/w
at constructor defaults, causing backend-dependent flakiness; before setting the
Poiseuille profile overwrite all velocity components to a known state (e.g.,
zero) by calling solver.velocity().u/v/w(...) or equivalent full-field setters
for all (i,j,k) indices, then apply the u-profile loop to overwrite u, and
finally call solver.sync_to_gpu(); ensure set_body_force(...) and
set_velocity_bc(...) remain as-is and that any turbulence model is set before
the first solver step (use RANSSolver::velocity(), RANSSolver::set_body_force,
and velocity().v/velocity().w identifiers to locate the changes).

---

Nitpick comments:
In `@tests/test_turbulence_cross_geometry.cpp`:
- Around line 829-844: The test suite touches GPU-initialization-sensitive cases
(see main() and the listed tests like test_3d_channel_smoke, test_3d_duct_smoke,
test_3d_tgv_smoke, test_dns_combo, test_cross_geometry_consistency), so before
merging run the GPU pre-CI locally: execute ./test_before_ci_gpu.sh with the
correct CUDA_VISIBLE_DEVICES and environment modules loaded, confirm the script
completes and that GPU vs CPU consistency and turbulence model GPU tests (the
ones invoked by run_sections) pass, and fix any GPU initialization failures
reported before pushing the merge.
- Around line 219-230: Extract the duplicated Taylor–Green vortex initialization
into a shared helper (e.g., initializeTaylorGreenVortex or initTGV) that accepts
the mesh and a reference to the solver.velocity() (or the velocity field object)
and performs the three nested loops that assign u and v using the same formulas
(u: sin(mesh.xf[i]) * cos(mesh.y(j)) * cos(mesh.z(k)); v: -cos(mesh.x(i)) *
sin(mesh.yf[j]) * cos(mesh.z(k))). Replace both duplicated blocks with calls to
this helper so both locations (including the other occurrence at lines 726-735)
use the same function and remove duplicated loop code.
- Around line 751-759: The test's "ordering" section only asserts E_baseline <=
E_laminar but mentions ordering for all schemes; update the check to either (A)
add an SST ordering assertion using the same pattern (e.g., add a record call
that asserts E_baseline <= E_sst && E_sst <= E_laminar or two comparisons with
the same ~1% tolerance) referencing the existing record function and variables
E_baseline, E_sst, E_laminar, or (B) if you only mean to compare baseline vs
laminar, rename the existing record description to something like "TGV:
E_baseline <= E_laminar" to reflect the narrower intent. Ensure you keep the
finite/positive energy check as-is.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0537872 and 72ec905.

📒 Files selected for processing (1)

• tests/test_turbulence_cross_geometry.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Initialize full velocity state before applying the DNS profile

Only u is explicitly assigned here. v/w currently depend on constructor defaults, which can make this test backend-dependent and flaky (especially with trip forcing touching w). Initialize the full field first, then overwrite u.

🔧 Proposed fix

         RANSSolver solver(mesh, config);
         solver.set_velocity_bc(create_velocity_bc(BCPattern::Channel3D));
         solver.set_body_force(0.001, 0.0, 0.0);
+        solver.initialize_uniform(0.0, 0.0);  // Deterministic v/w initialization before custom u IC
 
         // Poiseuille + perturbation
         for (int k = mesh.k_begin(); k < mesh.k_end(); ++k)
             for (int j = mesh.j_begin(); j < mesh.j_end(); ++j)
                 for (int i = mesh.i_begin(); i <= mesh.i_end(); ++i) {

As per coding guidelines: "Always call set_body_force() for driven flows; initialize velocity field before solving; set turbulence model before first solver step".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_turbulence_cross_geometry.cpp` around lines 295 - 306, The test
initializes only the u-component and leaves v/w at constructor defaults, causing
backend-dependent flakiness; before setting the Poiseuille profile overwrite all
velocity components to a known state (e.g., zero) by calling
solver.velocity().u/v/w(...) or equivalent full-field setters for all (i,j,k)
indices, then apply the u-profile loop to overwrite u, and finally call
solver.sync_to_gpu(); ensure set_body_force(...) and set_velocity_bc(...) remain
as-is and that any turbulence model is set before the first solver step (use
RANSSolver::velocity(), RANSSolver::set_body_force, and
velocity().v/velocity().w identifiers to locate the changes).

[coderabbitai]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

CMakeLists.txt (1)

481-505: ⚠️ Potential issue | 🟠 Major

GPU-only validation tests are registered unconditionally.

These tests are labeled gpu but still added in CPU builds. Since check-quick does not exclude gpu, they can run unexpectedly on non-GPU configurations.

🔧 Suggested fix

-    add_nncfd_test(test_tgv_validation TEST_NAME_SUFFIX TGVValidationTest LABELS "gpu;medium")
-    set_tests_properties(TGVValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+    if(USE_GPU_OFFLOAD)
+        add_nncfd_test(test_tgv_validation TEST_NAME_SUFFIX TGVValidationTest LABELS "gpu;medium")
+        set_tests_properties(TGVValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+    endif()
@@
-    add_nncfd_test(test_poiseuille_validation TEST_NAME_SUFFIX PoiseuilleValidationTest LABELS "gpu;medium")
-    set_tests_properties(PoiseuilleValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
-    add_nncfd_test(test_rans_channel_validation TEST_NAME_SUFFIX RANSChannelValidationTest LABELS "gpu;medium")
-    set_tests_properties(RANSChannelValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
-    add_nncfd_test(test_dns_channel_validation TEST_NAME_SUFFIX DNSChannelValidationTest LABELS "gpu;medium")
-    set_tests_properties(DNSChannelValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+    if(USE_GPU_OFFLOAD)
+        add_nncfd_test(test_poiseuille_validation TEST_NAME_SUFFIX PoiseuilleValidationTest LABELS "gpu;medium")
+        set_tests_properties(PoiseuilleValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+        add_nncfd_test(test_rans_channel_validation TEST_NAME_SUFFIX RANSChannelValidationTest LABELS "gpu;medium")
+        set_tests_properties(RANSChannelValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+        add_nncfd_test(test_dns_channel_validation TEST_NAME_SUFFIX DNSChannelValidationTest LABELS "gpu;medium")
+        set_tests_properties(DNSChannelValidationTest PROPERTIES ENVIRONMENT "OMP_TARGET_OFFLOAD=MANDATORY")
+    endif()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CMakeLists.txt` around lines 481 - 505, Several tests (TGVValidationTest,
PoiseuilleValidationTest, RANSChannelValidationTest, DNSChannelValidationTest)
are registered unconditionally even though they are GPU-only (label "gpu"); wrap
the add_nncfd_test and associated set_tests_properties calls for these
gpu-labeled tests in a CMake conditional that checks the project's GPU build
flag (e.g., if(NNCFD_ENABLE_GPU) ... endif or the existing CUDA/OpenMP offload
option used by the project) so they are only added when GPU support is enabled,
and ensure the set_tests_properties calls remain inside that same conditional
block.

🧹 Nitpick comments (4)

tests/test_tgv_validation.cpp (1)

99-111: Per-step host synchronization is expensive in GPU-labeled tests.

Calling sync_from_gpu() every iteration introduces heavy transfer overhead. Prefer device-side diagnostics each step (or every N steps) and sparse host sync.

As per coding guidelines, "Minimize CPU↔GPU transfers and keep frequently accessed data on GPU; batch operations when possible".

Also applies to: 175-190

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_tgv_validation.cpp` around lines 99 - 111, The test currently
calls solver.sync_from_gpu() on every iteration causing expensive CPU/GPU
transfers; change the loop to run per-step diagnostics on the device (add/use
solver methods that compute kinetic energy and max divergence on-GPU) or perform
host sync only every N steps (e.g., if (step % N == 0) { solver.sync_from_gpu();
read E and div; } ), updating E_history, energy_monotonic (E_prev) and max_div
only when a host read happens; reference symbols: solver.step(),
solver.sync_from_gpu(), compute_kinetic_energy_3d, compute_max_divergence_3d,
E_history, energy_monotonic, E_prev, max_div.

scripts/generate_validation_report.py (1)

148-149: Make zip() strictness explicit (B905).

Line 148 and Line 175 use zip(...) without strict=, which can silently truncate on length mismatch and triggers Ruff B905.

♻️ Suggested fix

-    for (name, prof), color in zip(sim_profiles.items(), colors):
+    for (name, prof), color in zip(sim_profiles.items(), colors, strict=True):
         ax.plot(prof["y_plus"], prof["u_plus"], "-", color=color, lw=1.2, label=name)

-    for ax, comp, label in zip(axes.flat, components, labels):
+    for ax, comp, label in zip(axes.flat, components, labels, strict=True):
         ref = mkm_stresses.get(comp)

Also applies to: 175-176

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/generate_validation_report.py` around lines 148 - 149, The zip usages
in the plotting loops (the for loop that iterates over "(name, prof), color in
zip(sim_profiles.items(), colors)" and the similar zip at lines ~175-176) can
silently truncate if lengths differ; change both zip(...) calls to use zip(...,
strict=True) so mismatched lengths raise a ValueError; update the two zip
invocations accordingly to satisfy Ruff B905 and ensure mismatches are caught
during execution.

docs/plans/2026-03-01-turbulence-validation-plan.md (1)

1678-1694: Add the GPU preflight script to final verification.

Before push, include ./test_before_ci_gpu.sh alongside fast/medium/new-test runs so GPU CI coverage is exercised the same way locally.

Based on learnings, "For GPU-related changes, also run ./test_before_ci_gpu.sh to validate GPU CI test suite locally including physics validation tests, turbulence model validation, and CPU/GPU consistency".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-03-01-turbulence-validation-plan.md` around lines 1678 -
1694, Add running the GPU preflight script to the final verification sequence by
invoking ./test_before_ci_gpu.sh before the existing fast/medium and new
validation tests; update the checklist that currently lists
test_poiseuille_validation, test_tgv_validation, test_dns_channel_validation,
and test_rans_channel_validation to run ./test_before_ci_gpu.sh (which exercises
GPU CI, physics validation, turbulence model validation, and CPU/GPU
consistency) as a required step prior to push.

tests/test_poiseuille_validation.cpp (1)

37-38: Use const-correct solver reference in the error helper.

compute_poiseuille_errors reads solver state only; this should take const RANSSolver&.

♻️ Suggested refactor

-ProfileErrors compute_poiseuille_errors(RANSSolver& solver, const Mesh& mesh,
+ProfileErrors compute_poiseuille_errors(const RANSSolver& solver, const Mesh& mesh,
                                          double dp_dx, double nu) {

As per coding guidelines, "Use `const` for read-only references and mark methods `const` if they don't modify state in C++ code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_poiseuille_validation.cpp` around lines 37 - 38, The helper
function compute_poiseuille_errors should take a const reference to the solver
since it only reads state: change its signature from RANSSolver& solver to const
RANSSolver& solver and update any calls accordingly; if the body calls non-const
RANSSolver methods, replace them with their const overloads or mark those
accessors const on the RANSSolver class so the function compiles and remains
const-correct.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/plans/2026-03-01-turbulence-validation-design.md`:
- Line 97: Update the documented Brachet reference filename to match the actual
generated asset: replace the table entry referencing
`data/reference/brachet_tgv/dissipation.dat` with the actual filename
`data/reference/brachet_tgv/dissipation_re1600.dat` (or, alternatively, change
the asset generation code to produce `dissipation.dat`); locate the table row
containing `data/reference/brachet_tgv/dissipation.dat` and update the string to
the correct generated name so the docs and workflow stay consistent.

In `@docs/plans/2026-03-01-turbulence-validation-plan.md`:
- Around line 1701-1704: Replace the hard-coded user-specific path "cd
/storage/scratch1/6/sbryngelson3/cfd-nn" with a portable reference (e.g. a
repository-relative path or an environment variable like $REPO_ROOT or $WORKDIR)
so collaborators can run the verification steps without editing the file; keep
the remainder of the steps (mkdir -p build_debug && cd build_debug, the cmake
invocation with -DCMAKE_CXX_COMPILER=nvc++ -DUSE_GPU_OFFLOAD=ON
-DCMAKE_BUILD_TYPE=Debug, and the make invocation for test_poiseuille_validation
test_tgv_validation test_dns_channel_validation test_rans_channel_validation)
unchanged and document that REPO_ROOT should point to the project root before
running these commands.

In `@scripts/generate_validation_report.py`:
- Around line 86-88: The profile discovery only searches one directory level in
find_profile_files; update it to search recursively (e.g., use glob with
recursive=True and a ** pattern or switch to pathlib.Path.rglob) so model/run
subdirectories are included; apply the same change to the other similar call
around the code referenced at lines 322-323 to ensure all profile lookups are
recursive.

In `@scripts/run_validation.sh`:
- Around line 35-39: The current check only tests for "$DATA_DIR/mkm_retau180"
and can skip downloading when other required assets like
"brachet_tgv/dissipation_re1600.dat" are missing; update the condition in
scripts/run_validation.sh to verify presence of all required reference assets
(e.g., test both "$DATA_DIR/mkm_retau180" and
"$DATA_DIR/brachet_tgv/dissipation_re1600.dat" or check for a manifest) and call
"$SCRIPT_DIR/download_reference_data.sh" "$DATA_DIR" when any required
directory/file is absent so the download runs whenever any needed asset is
missing.

In `@tests/test_dns_channel_validation.cpp`:
- Around line 30-35: Add a runtime GPU-device check after the existing `#else` so
the test skips cleanly when no GPU is present: call gpu::available() (provided
by test_utilities.hpp) and if it returns false, print the same skip message,
call record("GPU build required", true, true) and return; keep the existing
compile-time guard (USE_GPU_OFFLOAD) intact and only perform this check inside
the `#else` branch so mis-scheduled nodes do not hit an unhelpful runtime error.

In `@tests/test_poiseuille_validation.cpp`:
- Around line 139-143: After each call to solver.solve_steady() (the occurrences
that assign auto [residual, iters]), add a test assertion that enforces
convergence by checking the residual is below an acceptable tolerance (e.g.
EXPECT_LT(residual, tol) or ASSERT_LT(residual, 1e-6)) and optionally assert
iteration bounds (e.g. EXPECT_LE(iters, max_iters) or ASSERT_LE(iters,
someReasonableLimit)); do this for the three places where auto [residual, iters]
= solver.solve_steady() is used so the test fails when the solver hasn't truly
converged rather than merely printing the post-hoc profile error.
- Around line 194-196: The fine-grid L2 error check is weaker than the
coarse-grid check; update the `record("Fine-grid L2 error < 2%", err.l2_rel <
0.02)` assertion so its threshold is stricter than the 32x64 section’s `< 0.01`
criterion (e.g. change the test to `err.l2_rel < 0.01`) and update the message
string to reflect the new bound; keep the `record("Fine-grid incompressibility",
err.max_div < 1e-6)` unchanged.

In `@tests/test_rans_channel_validation.cpp`:
- Around line 145-152: The current check uses result.max_nut > 0 to flag
positivity but misses negative values; change the validation to compute the
minimum eddy viscosity (e.g., introduce result.min_nut and set it by iterating
over solver.nu_t()(i, j) alongside max) and set result.nut_positive =
(result.min_nut >= 0.0) (or require both min >= 0 and max > 0 if you want
nonzero positive); update the loops that now reference result.max_nut and
solver.nu_t() to also compute result.min_nut so the test fails if any cell has
negative nu_t.
- Around line 329-356: The test computes the law-of-wall metric (variables
sublayer_err and n_sublayer using mesh, solver.velocity().u and solver.nu_t())
but never asserts it; add an explicit validation that there were sublayer points
and that the relative error is within an acceptable tolerance (e.g., require
n_sublayer > 0 and sublayer_err < tolerance) by recording or asserting it (for
example via record("Baseline: law-of-wall sublayer valid", n_sublayer > 0 &&
sublayer_err < 0.2)) so the test will fail if the viscous sublayer behavior is
not met.

In `@tests/test_tgv_validation.cpp`:
- Around line 113-117: The test computes an analytical energy decay
(E_analytical_approx) but never asserts it against the simulated value (E_final
from E_history); add a validation after computing E_analytical_approx: compare
E_final to E_analytical_approx with a reasonable relative tolerance (e.g., using
the test framework's near-equality assertion like REQUIRE_NEAR/ASSERT_NEAR) and
fail the test if the difference exceeds that tolerance; apply the same fix for
the later block around E_analytical_approx/E_final at lines 129-135 so both
early-time checks enforce the expected exponential decay (refer to symbols
E_analytical_approx, E_final, E_history, E_initial, nsteps, dt, nu).
- Around line 132-134: The three test assertions using record(...) with absolute
tolerance 1e-10 (the calls referencing mean_vel.u, mean_vel.v, mean_vel.w) are
too strict for cross-platform GPU/CI; relax them to a more realistic
floating-point tolerance (e.g., 1e-8 or algorithm-appropriate) or switch to a
relative check based on the magnitude of mean_vel components so the tests are
robust to reduction/FP variation.

---

Outside diff comments:
In `@CMakeLists.txt`:
- Around line 481-505: Several tests (TGVValidationTest,
PoiseuilleValidationTest, RANSChannelValidationTest, DNSChannelValidationTest)
are registered unconditionally even though they are GPU-only (label "gpu"); wrap
the add_nncfd_test and associated set_tests_properties calls for these
gpu-labeled tests in a CMake conditional that checks the project's GPU build
flag (e.g., if(NNCFD_ENABLE_GPU) ... endif or the existing CUDA/OpenMP offload
option used by the project) so they are only added when GPU support is enabled,
and ensure the set_tests_properties calls remain inside that same conditional
block.

---

Nitpick comments:
In `@docs/plans/2026-03-01-turbulence-validation-plan.md`:
- Around line 1678-1694: Add running the GPU preflight script to the final
verification sequence by invoking ./test_before_ci_gpu.sh before the existing
fast/medium and new validation tests; update the checklist that currently lists
test_poiseuille_validation, test_tgv_validation, test_dns_channel_validation,
and test_rans_channel_validation to run ./test_before_ci_gpu.sh (which exercises
GPU CI, physics validation, turbulence model validation, and CPU/GPU
consistency) as a required step prior to push.

In `@scripts/generate_validation_report.py`:
- Around line 148-149: The zip usages in the plotting loops (the for loop that
iterates over "(name, prof), color in zip(sim_profiles.items(), colors)" and the
similar zip at lines ~175-176) can silently truncate if lengths differ; change
both zip(...) calls to use zip(..., strict=True) so mismatched lengths raise a
ValueError; update the two zip invocations accordingly to satisfy Ruff B905 and
ensure mismatches are caught during execution.

In `@tests/test_poiseuille_validation.cpp`:
- Around line 37-38: The helper function compute_poiseuille_errors should take a
const reference to the solver since it only reads state: change its signature
from RANSSolver& solver to const RANSSolver& solver and update any calls
accordingly; if the body calls non-const RANSSolver methods, replace them with
their const overloads or mark those accessors const on the RANSSolver class so
the function compiles and remains const-correct.

In `@tests/test_tgv_validation.cpp`:
- Around line 99-111: The test currently calls solver.sync_from_gpu() on every
iteration causing expensive CPU/GPU transfers; change the loop to run per-step
diagnostics on the device (add/use solver methods that compute kinetic energy
and max divergence on-GPU) or perform host sync only every N steps (e.g., if
(step % N == 0) { solver.sync_from_gpu(); read E and div; } ), updating
E_history, energy_monotonic (E_prev) and max_div only when a host read happens;
reference symbols: solver.step(), solver.sync_from_gpu(),
compute_kinetic_energy_3d, compute_max_divergence_3d, E_history,
energy_monotonic, E_prev, max_div.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72ec905 and 21e70d1.

📒 Files selected for processing (11)

• CMakeLists.txt
• docs/plans/2026-03-01-turbulence-validation-design.md
• docs/plans/2026-03-01-turbulence-validation-plan.md
• scripts/download_reference_data.sh
• scripts/generate_validation_report.py
• scripts/run_validation.sh
• tests/reference_data.hpp
• tests/test_dns_channel_validation.cpp
• tests/test_poiseuille_validation.cpp
• tests/test_rans_channel_validation.cpp
• tests/test_tgv_validation.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Brachet reference filename is inconsistent with the actual generated asset.

Line 97 documents dissipation.dat, but the workflow generates dissipation_re1600.dat.

📝 Suggested fix

-| `data/reference/brachet_tgv/dissipation.dat` | TGV reference curve | data |
+| `data/reference/brachet_tgv/dissipation_re1600.dat` | TGV reference curve | data |

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	| `data/reference/brachet_tgv/dissipation.dat` | TGV reference curve | data |
	| `data/reference/brachet_tgv/dissipation_re1600.dat` | TGV reference curve | data |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-03-01-turbulence-validation-design.md` at line 97, Update the
documented Brachet reference filename to match the actual generated asset:
replace the table entry referencing `data/reference/brachet_tgv/dissipation.dat`
with the actual filename `data/reference/brachet_tgv/dissipation_re1600.dat`
(or, alternatively, change the asset generation code to produce
`dissipation.dat`); locate the table row containing
`data/reference/brachet_tgv/dissipation.dat` and update the string to the
correct generated name so the docs and workflow stay consistent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Avoid user-specific absolute paths in verification steps.

Line 1701 hard-codes a personal filesystem path, which makes the runbook non-portable for collaborators.

📌 Suggested docs fix

-cd /storage/scratch1/6/sbryngelson3/cfd-nn
+cd /path/to/cfd-nn

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	cd /storage/scratch1/6/sbryngelson3/cfd-nn
	mkdir -p build_debug && cd build_debug
	cmake .. -DCMAKE_CXX_COMPILER=nvc++ -DUSE_GPU_OFFLOAD=ON -DCMAKE_BUILD_TYPE=Debug
	make -j$(nproc) test_poiseuille_validation test_tgv_validation test_dns_channel_validation test_rans_channel_validation
	cd /path/to/cfd-nn
	mkdir -p build_debug && cd build_debug
	cmake .. -DCMAKE_CXX_COMPILER=nvc++ -DUSE_GPU_OFFLOAD=ON -DCMAKE_BUILD_TYPE=Debug
	make -j$(nproc) test_poiseuille_validation test_tgv_validation test_dns_channel_validation test_rans_channel_validation

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-03-01-turbulence-validation-plan.md` around lines 1701 -
1704, Replace the hard-coded user-specific path "cd
/storage/scratch1/6/sbryngelson3/cfd-nn" with a portable reference (e.g. a
repository-relative path or an environment variable like $REPO_ROOT or $WORKDIR)
so collaborators can run the verification steps without editing the file; keep
the remainder of the steps (mkdir -p build_debug && cd build_debug, the cmake
invocation with -DCMAKE_CXX_COMPILER=nvc++ -DUSE_GPU_OFFLOAD=ON
-DCMAKE_BUILD_TYPE=Debug, and the make invocation for test_poiseuille_validation
test_tgv_validation test_dns_channel_validation test_rans_channel_validation)
unchanged and document that REPO_ROOT should point to the project root before
running these commands.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Profile discovery should be recursive.

Current discovery only scans one directory level, so model/run subdirectories can be skipped and the report will incorrectly show no profiles found.

✅ Suggested fix

 def find_profile_files(output_dir, pattern):
     """Find simulation output files matching a glob pattern."""
-    return sorted(glob.glob(os.path.join(output_dir, pattern)))
+    return sorted(glob.glob(os.path.join(output_dir, "**", pattern), recursive=True))

Also applies to: 322-323

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/generate_validation_report.py` around lines 86 - 88, The profile
discovery only searches one directory level in find_profile_files; update it to
search recursively (e.g., use glob with recursive=True and a ** pattern or
switch to pathlib.Path.rglob) so model/run subdirectories are included; apply
the same change to the other similar call around the code referenced at lines
322-323 to ensure all profile lookups are recursive.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Incomplete reference-data check can skip required TGV assets.

Line 36 only checks mkm_retau180. If MKM exists but brachet_tgv/dissipation_re1600.dat is missing, download is skipped and downstream reporting can fail.

🔧 Suggested fix

-# Download reference data if not present
-if [ ! -d "$DATA_DIR/mkm_retau180" ]; then
+# Download reference data if required assets are missing
+if [ ! -f "$DATA_DIR/mkm_retau180/chan180_means.dat" ] || \
+   [ ! -f "$DATA_DIR/brachet_tgv/dissipation_re1600.dat" ]; then
     echo "Downloading reference data..."
     "$SCRIPT_DIR/download_reference_data.sh" "$DATA_DIR"
 fi

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# Download reference data if not present
	if [ ! -d "$DATA_DIR/mkm_retau180" ]; then
	echo "Downloading reference data..."
	"$SCRIPT_DIR/download_reference_data.sh" "$DATA_DIR"
	fi
	# Download reference data if required assets are missing
	if [ ! -f "$DATA_DIR/mkm_retau180/chan180_means.dat" ] || \
	[ ! -f "$DATA_DIR/brachet_tgv/dissipation_re1600.dat" ]; then
	echo "Downloading reference data..."
	"$SCRIPT_DIR/download_reference_data.sh" "$DATA_DIR"
	fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/run_validation.sh` around lines 35 - 39, The current check only tests
for "$DATA_DIR/mkm_retau180" and can skip downloading when other required assets
like "brachet_tgv/dissipation_re1600.dat" are missing; update the condition in
scripts/run_validation.sh to verify presence of all required reference assets
(e.g., test both "$DATA_DIR/mkm_retau180" and
"$DATA_DIR/brachet_tgv/dissipation_re1600.dat" or check for a manifest) and call
"$SCRIPT_DIR/download_reference_data.sh" "$DATA_DIR" when any required
directory/file is absent so the download runs whenever any needed asset is
missing.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify GPU offload guard presence and state in the test file and CMakeLists.txt
rg -n -C2 'USE_GPU_OFFLOAD|omp_get_num_devices|OMP_TARGET_OFFLOAD' tests/test_dns_channel_validation.cpp CMakeLists.txt

Repository: comp-physics/cfd-nn

Length of output: 7341

---

🏁 Script executed:

#!/bin/bash
# Check includes in the test file and see how other GPU tests handle runtime device checks
head -n 30 tests/test_dns_channel_validation.cpp

echo "===== Searching other GPU tests for omp_get_num_devices pattern ====="
rg -l 'omp_get_num_devices' tests/

Repository: comp-physics/cfd-nn

Length of output: 1316

---

🏁 Script executed:

#!/bin/bash
# Check how other GPU tests implement runtime device checks
echo "===== test_gpu_turbulence_readiness.cpp ====="
rg -B5 -A5 'omp_get_num_devices' tests/test_gpu_turbulence_readiness.cpp | head -40

echo "===== test_utilities.hpp ====="
rg -B5 -A5 'omp_get_num_devices' tests/test_utilities.hpp | head -40

echo "===== test_harness.hpp includes ====="
head -n 50 tests/test_harness.hpp | grep -E '#include|omp'

Repository: comp-physics/cfd-nn

Length of output: 1969

---

Add runtime GPU-device check using the available utility function.

This test has a compile-time guard but no runtime device check. The codebase already provides gpu::available() in test_utilities.hpp (which is included via test_harness.hpp). Use it to gracefully skip the test when no GPU devices are present, rather than relying on OMP_TARGET_OFFLOAD=MANDATORY to fail with an unhelpful runtime error on mis-scheduled nodes.

🔧 Suggested fix

 `#ifndef` USE_GPU_OFFLOAD
     std::cout << "  [SKIP] DNS channel test requires GPU build\n\n";
     record("GPU build required", true, true);  // skip
     return;
 `#else`
+    if (!gpu::available()) {
+        std::cout << "  [SKIP] No OpenMP target devices available\n\n";
+        record("GPU runtime available", true, true);  // skip
+        return;
+    }

Per coding guidelines: "For GPU offload, verify USE_GPU_OFFLOAD is defined and check omp_get_num_devices() > 0 at runtime".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	#ifndef USE_GPU_OFFLOAD
	std::cout << " [SKIP] DNS channel test requires GPU build\n\n";
	record("GPU build required", true, true); // skip
	return;
	#else
	`#ifndef` USE_GPU_OFFLOAD
	std::cout << " [SKIP] DNS channel test requires GPU build\n\n";
	record("GPU build required", true, true); // skip
	return;
	`#else`
	if (!gpu::available()) {
	std::cout << " [SKIP] No OpenMP target devices available\n\n";
	record("GPU runtime available", true, true); // skip
	return;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_dns_channel_validation.cpp` around lines 30 - 35, Add a runtime
GPU-device check after the existing `#else` so the test skips cleanly when no GPU
is present: call gpu::available() (provided by test_utilities.hpp) and if it
returns false, print the same skip message, call record("GPU build required",
true, true) and return; keep the existing compile-time guard (USE_GPU_OFFLOAD)
intact and only perform this check inside the `#else` branch so mis-scheduled
nodes do not hit an unhelpful runtime error.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fine-grid criterion is weaker than coarse-grid criterion.

Line 195 says finer mesh should improve accuracy, but err.l2_rel < 0.02 is looser than the 32x64 section’s < 0.01, so refinement regressions can still pass.

✅ Suggested fix

-    record("Fine-grid L2 error < 2%", err.l2_rel < 0.02);
+    record("Fine-grid L2 error < 1%", err.l2_rel < 0.01);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Finer grid should give smaller error
	record("Fine-grid L2 error < 2%", err.l2_rel < 0.02);
	record("Fine-grid incompressibility", err.max_div < 1e-6);
	// Finer grid should give smaller error
	record("Fine-grid L2 error < 1%", err.l2_rel < 0.01);
	record("Fine-grid incompressibility", err.max_div < 1e-6);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_poiseuille_validation.cpp` around lines 194 - 196, The fine-grid
L2 error check is weaker than the coarse-grid check; update the
`record("Fine-grid L2 error < 2%", err.l2_rel < 0.02)` assertion so its
threshold is stricter than the 32x64 section’s `< 0.01` criterion (e.g. change
the test to `err.l2_rel < 0.01`) and update the message string to reflect the
new bound; keep the `record("Fine-grid incompressibility", err.max_div < 1e-6)`
unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

nu_t positivity check can miss negative eddy viscosity.

max_nut > 0 passes even if parts of the field are negative. This does not validate realizability.

🔧 Suggested fix

-        // Max nu_t
-        result.max_nut = 0.0;
+        // nu_t realizability
+        result.max_nut = 0.0;
+        bool nut_nonnegative = true;
+        bool nut_has_positive = false;
         for (int j = mesh.j_begin(); j < mesh.j_end(); ++j) {
             for (int i = mesh.i_begin(); i < mesh.i_end(); ++i) {
-                result.max_nut = std::max(result.max_nut, solver.nu_t()(i, j));
+                double nut = solver.nu_t()(i, j);
+                result.max_nut = std::max(result.max_nut, nut);
+                if (nut < 0.0) nut_nonnegative = false;
+                if (nut > 0.0) nut_has_positive = true;
             }
         }
-        result.nut_positive = result.max_nut > 0.0;
+        result.nut_positive = nut_nonnegative && nut_has_positive;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_rans_channel_validation.cpp` around lines 145 - 152, The current
check uses result.max_nut > 0 to flag positivity but misses negative values;
change the validation to compute the minimum eddy viscosity (e.g., introduce
result.min_nut and set it by iterating over solver.nu_t()(i, j) alongside max)
and set result.nut_positive = (result.min_nut >= 0.0) (or require both min >= 0
and max > 0 if you want nonzero positive); update the loops that now reference
result.max_nut and solver.nu_t() to also compute result.min_nut so the test
fails if any cell has negative nu_t.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Law-of-wall metric is computed but never enforced.

sublayer_err and n_sublayer are printed only. The section can pass without validating the claimed sublayer behavior.

🔧 Suggested fix

     record("Baseline converged", residual < 1e-3);
     record("Baseline: Re_tau > 100", re_tau > 100.0);
+    record("Baseline: sublayer points present", n_sublayer > 0);
+    record("Baseline: viscous sublayer error < 25%", n_sublayer > 0 && sublayer_err < 0.25);
     record("Baseline: nu_t > 0", max_nut > 0.0);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Check viscous sublayer (y+ < 5: U+ ≈ y+)
	int i_mid = mesh.i_begin() + mesh.Nx / 2;
	double sublayer_err = 0.0;
	int n_sublayer = 0;
	double max_nut = 0.0;
	for (int j = mesh.j_begin(); j < mesh.j_begin() + mesh.Ny / 2; ++j) {
	double y = mesh.y(j) - mesh.y_min;
	double y_plus = y * u_tau / nu;
	double u_num = 0.5 * (solver.velocity().u(i_mid, j) + solver.velocity().u(i_mid + 1, j));
	double u_plus = u_num / u_tau;
	if (y_plus < 5.0 && y_plus > 0.1) {
	sublayer_err = std::max(sublayer_err, std::abs(u_plus - y_plus) / y_plus);
	++n_sublayer;
	}
	max_nut = std::max(max_nut, solver.nu_t()(i_mid, j));
	}
	std::cout << " Sublayer max rel error: " << std::scientific << sublayer_err
	<< " (n_points=" << n_sublayer << ")\n";
	std::cout << " max nu_t: " << max_nut << "\n\n";
	record("Baseline converged", residual < 1e-3);
	record("Baseline: Re_tau > 100", re_tau > 100.0);
	record("Baseline: nu_t > 0", max_nut > 0.0);
	}
	// Check viscous sublayer (y+ < 5: U+ ≈ y+)
	int i_mid = mesh.i_begin() + mesh.Nx / 2;
	double sublayer_err = 0.0;
	int n_sublayer = 0;
	double max_nut = 0.0;
	for (int j = mesh.j_begin(); j < mesh.j_begin() + mesh.Ny / 2; ++j) {
	double y = mesh.y(j) - mesh.y_min;
	double y_plus = y * u_tau / nu;
	double u_num = 0.5 * (solver.velocity().u(i_mid, j) + solver.velocity().u(i_mid + 1, j));
	double u_plus = u_num / u_tau;
	if (y_plus < 5.0 && y_plus > 0.1) {
	sublayer_err = std::max(sublayer_err, std::abs(u_plus - y_plus) / y_plus);
	+n_sublayer;
	}
	max_nut = std::max(max_nut, solver.nu_t()(i_mid, j));
	}
	std::cout << " Sublayer max rel error: " << std::scientific << sublayer_err
	<< " (n_points=" << n_sublayer << ")\n";
	std::cout << " max nu_t: " << max_nut << "\n\n";
	record("Baseline converged", residual < 1e-3);
	record("Baseline: Re_tau > 100", re_tau > 100.0);
	record("Baseline: sublayer points present", n_sublayer > 0);
	record("Baseline: viscous sublayer error < 25%", n_sublayer > 0 && sublayer_err < 0.25);
	record("Baseline: nu_t > 0", max_nut > 0.0);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_rans_channel_validation.cpp` around lines 329 - 356, The test
computes the law-of-wall metric (variables sublayer_err and n_sublayer using
mesh, solver.velocity().u and solver.nu_t()) but never asserts it; add an
explicit validation that there were sublayer points and that the relative error
is within an acceptable tolerance (e.g., require n_sublayer > 0 and sublayer_err
< tolerance) by recording or asserting it (for example via record("Baseline:
law-of-wall sublayer valid", n_sublayer > 0 && sublayer_err < 0.2)) so the test
will fail if the viscous sublayer behavior is not met.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Analytical decay is computed but never validated.

E_analytical_approx is logged but not used in any assertion, so the claimed early-time analytical check is not enforced.

🔧 Suggested fix

     double E_analytical_approx = E_initial * std::exp(-2.0 * nu * t_final);
     double E_final = E_history.back();
     double decay_ratio = E_final / E_initial;
+    double rel_err_analytical =
+        std::abs(E_final - E_analytical_approx) / std::max(1e-12, std::abs(E_analytical_approx));
@@
     record("Energy monotonically decays", energy_monotonic);
     record("Energy decayed (ratio < 0.99)", decay_ratio < 0.99);
+    record("Early-time analytical decay (rel err < 20%)", rel_err_analytical < 0.20);

Also applies to: 129-135

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_tgv_validation.cpp` around lines 113 - 117, The test computes an
analytical energy decay (E_analytical_approx) but never asserts it against the
simulated value (E_final from E_history); add a validation after computing
E_analytical_approx: compare E_final to E_analytical_approx with a reasonable
relative tolerance (e.g., using the test framework's near-equality assertion
like REQUIRE_NEAR/ASSERT_NEAR) and fail the test if the difference exceeds that
tolerance; apply the same fix for the later block around
E_analytical_approx/E_final at lines 129-135 so both early-time checks enforce
the expected exponential decay (refer to symbols E_analytical_approx, E_final,
E_history, E_initial, nsteps, dt, nu).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Symmetry tolerance is likely too strict for stable cross-platform CI.

Absolute 1e-10 on mean velocity can be flaky for GPU reductions and compiler/runtime variation.

🔧 Suggested fix

-    record("Symmetry <u>~0 (< 1e-10)", std::abs(mean_vel.u) < 1e-10);
-    record("Symmetry <v>~0 (< 1e-10)", std::abs(mean_vel.v) < 1e-10);
-    record("Symmetry <w>~0 (< 1e-10)", std::abs(mean_vel.w) < 1e-10);
+    const double sym_tol = 1e-8;
+    record("Symmetry <u>~0 (< 1e-8)", std::abs(mean_vel.u) < sym_tol);
+    record("Symmetry <v>~0 (< 1e-8)", std::abs(mean_vel.v) < sym_tol);
+    record("Symmetry <w>~0 (< 1e-8)", std::abs(mean_vel.w) < sym_tol);

As per coding guidelines, "Avoid overly strict floating-point comparisons in tests; use appropriate tolerances based on algorithm rather than exact equality checks (e.g., avoid `==` for doubles)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_tgv_validation.cpp` around lines 132 - 134, The three test
assertions using record(...) with absolute tolerance 1e-10 (the calls
referencing mean_vel.u, mean_vel.v, mean_vel.w) are too strict for
cross-platform GPU/CI; relax them to a more realistic floating-point tolerance
(e.g., 1e-8 or algorithm-appropriate) or switch to a relative check based on the
magnitude of mean_vel components so the tests are robust to reduction/FP
variation.

[qodo-code-review]

CI Feedback 🧐

(Feedback updated until commit 63b7799)

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: build-and-test (ubuntu-latest, Debug)

Failed stage: Build (CPU-only) [❌]

Failed test name: ""

Failure summary: The action failed during CMake configuration because the build system attempted to create a duplicate executable target. - CMake error at CMakeLists.txt:505 (add_executable): target verify_implicit_ydiff could not be created because another target with the same name already exists. - The duplicate target originates from the same source directory (/home/runner/work/cfd-nn/cfd-nn), and was triggered via CMakeLists.txt:716 (add_nncfd_test). - Because of this CMake target name collision (policy CMP0002), configuration was incomplete and the job exited with code 1.

Relevant error logs: 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 248: shell: /usr/bin/bash -e {0} 249: ##[endgroup] 250: ========================================= 251: Building CPU-only version (Debug) 252: ========================================= 253: Note: This CI workflow tests CPU-only builds. 254: GPU offload is tested separately in gpu-ci.yml workflow. 255: -- The CXX compiler identification is GNU 13.3.0 256: -- Detecting CXX compiler ABI info 257: -- Detecting CXX compiler ABI info - done 258: -- Check for working CXX compiler: /usr/bin/c++ - skipped 259: -- Detecting CXX compile features 260: -- Detecting CXX compile features - done 261: -- CPU build (OpenMP disabled, single-threaded) 262: -- Suppressing unknown pragma warnings for CPU build 263: CMake Error at CMakeLists.txt:505 (add_executable): 264: add_executable cannot create target "verify_implicit_ydiff" because another 265: target with the same name already exists. The existing target is an 266: executable created in source directory "/home/runner/work/cfd-nn/cfd-nn". 267: See documentation for policy CMP0002 for more details. 268: Call Stack (most recent call first): 269: CMakeLists.txt:716 (add_nncfd_test) 270: -- Configuring incomplete, errors occurred! 271: ##[error]Process completed with exit code 1. 272: Post job cleanup.