Repository navigation
Reapply 3+1D CDT support - #138
rakovpublic wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a complete 4D CDT stack. It defines foliated triangulations, seven moves, action and detailed-balance calculations, Metropolis sampling, checkpoints, structured output, phase diagnostics, CLI support, scan scripts, and tests. Changes4D CDT implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Simulation4D
participant Metropolis4
participant FoliatedTriangulation4
participant SimulationOutput
CLI->>Simulation4D: validate and construct 4D configuration
Simulation4D->>Metropolis4: provide sampler configuration
CLI->>Metropolis4: run seeded chain
Metropolis4->>FoliatedTriangulation4: apply moves and measure state
FoliatedTriangulation4-->>Metropolis4: return updated state
Metropolis4-->>CLI: return simulation result
CLI->>SimulationOutput: write_run_directory
SimulationOutput-->>CLI: write measurements and summary
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 47
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Phase_analysis_test.cpp (1)
1-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
estimate_autocorrelation_timeandto_string.No
TEST_CASEexercisesestimate_autocorrelation_time(constant series, positive/negative correlated series, short series below the 3-sample floor) orto_string(round-trip over eachVerdict). Both are part of the public header surface. Add direct tests for these two functions to close the coverage gap.As per path instructions for
tests/**/*.cpp: "Comprehensive test coverage and proper test organization."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Phase_analysis_test.cpp` around lines 1 - 105, Add direct TEST_CASEs for estimate_autocorrelation_time covering constant input, positively and negatively correlated series, and series shorter than the three-sample minimum, asserting the documented results. Add a to_string round-trip test that exercises every Verdict enum value and verifies each converted string maps to the expected value or representation. Keep these tests organized alongside the existing phase-analysis coverage.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Detailed_balance_4.hpp`:
- Around line 71-102: Add a max_states parameter to verify_detailed_balance and
enforce it while inserting newly discovered states; when the cap is reached,
stop enumeration and record an explicit error in DetailedBalanceReport4D instead
of allocating or processing further states. Preserve the existing depth limit
and traversal behavior below the cap, and use the report’s established error
mechanism.
- Around line 129-145: Update the detailed-balance verification around
boltzmann_weight, lhs, and rhs to detect non-finite or underflowed weights and
mark report.passed false with an error instead of allowing NaN or zero residuals
to pass. Prefer a log-space residual/comparison for finite values to avoid
overflow. Also document that the current lhs/rhs comparison is an algebraic
identity because both acceptance_probability calls apply the Metropolis-Hastings
ratio, or replace it with a comparison against the acceptance rule used by
Metropolis_4.
In `@include/Ergodic_moves_4.hpp`:
- Around line 33-38: In the 4D move application flow, check that `forward` is
non-zero before creating `candidate`, so rejected proposals avoid constructing a
copy. When constructing `MoveApplication`, move the named `candidate` into it
rather than copying it, while preserving the existing applicability validation
and reverse-move handling.
In `@include/Foliated_triangulation_4.hpp`:
- Around line 581-605: Update apply_move so each successfully applied move
synchronizes m_spatial_profile, m_vertices, and m_simplices with the resulting
triangulation, using the move descriptor’s per-slice deltas and existing
complex-update mechanisms. Ensure validation rollback restores these fields
alongside the existing state, and keep profile-derived observables and
canonical_hash() consistent with the post-move state.
- Around line 760-770: Update canonical_hash() to include m_three_three_forward
in the serialized state identity, alongside the existing m_timeslices and
m_periodic flags. Preserve the current counts and spatial-profile serialization
so states differing only in this direction flag produce distinct keys.
- Around line 351-371: Clamp timeslices to a minimum of 2 in the
FoliatedTriangulation4 counts constructor before storing or using it, matching
periodic_seed behavior. Ensure m_timeslices, profile initialization, and
slice_euler_characteristics() all use the clamped value, including calls through
from_counts_for_validation.
- Around line 425-447: Update spatial_topology() and
slice_euler_characteristics() so they report validated topology rather than
unconditional S3 declarations. Derive each slice’s Euler characteristic from its
spacelike facets, and add connectivity validation over Simplex4D::neighbors in
validate() so non-S3 slices are rejected; do not rely solely on
m_closed_s3_slices or the literal accessor value.
- Around line 88-93: Replace the linear search in vertex_time with an
unordered_map lookup using a new m_vertex_times cache keyed by VertexId. Update
this cache whenever vertices change, specifically in add_vertex and
time_reversed, while preserving -1 for missing vertices and keeping cached times
synchronized with m_vertices.
- Around line 726-758: Update FoliatedTriangulation4::time_reversed so
m_spatial_profile uses the same cyclic time mapping as vertex.time: preserve the
profile entry for slice 0 and shift each other slice from i to (m_timeslices -
i) % m_timeslices instead of applying std::reverse. Also invert
m_three_three_forward in the returned triangulation so candidate_multiplicity
observes the reversed proposal direction.
In `@include/Metropolis_4.hpp`:
- Line 69: Replace the raw pcg64 member m_rng in the Metropolis4 sampler with
cdt::Random, and initialize it from a dedicated random_streams::transitions
stream derived using chain_id. Update the 4D construction path and
Metropolis4Config seed handling so the full stream identity is preserved rather
than passing only root_random.seed().value(), ensuring different chain_id values
produce distinct replayable streams.
- Around line 71-75: Update propose_move to derive the distribution’s upper
bound from move_tracker::NUMBER_OF_4D_MOVES instead of the hardcoded 6,
preserving the existing inclusive range expected by move_tracker::as_move_4d.
- Around line 140-185: Update Metropolis4::run around the main step loop to
honor Metropolis4Config::checkpoint_interval by writing a checkpoint at each
configured interval, after measurements are processed. Reuse the existing
checkpoint-saving mechanism and ensure checkpointing is disabled when the
interval is non-positive; preserve the current final checkpoint behavior.
- Around line 204-225: Update load_checkpoint to validate the state.txt stream
after each extraction group, including the RNG state line, and throw an
appropriate exception on any missing or malformed data instead of constructing a
fallback triangulation. Add the required stdexcept include and ensure
set_rng_state is only called after successfully reading a non-failed RNG state.
- Around line 187-225: Update save_checkpoint and load_checkpoint so checkpoints
preserve and restore the complete FoliatedTriangulation4 topology, including
vertices and simplices, rather than reconstructing only from timeslices, counts,
and profile. Ensure load_checkpoint returns a fully populated complex with
consistent combinatorial data for move and candidate APIs, while preserving step
and RNG state restoration.
- Around line 187-202: Update save_checkpoint to include the required standard
exception header and validate the ofstream both after opening and after
completing all writes; if either check fails, throw an appropriate exception so
callers do not report an unsuccessful checkpoint as written. Prefer writing to a
temporary file and atomically renaming it to state.txt after successful
completion to avoid torn checkpoints.
In `@include/Move_catalog_4.hpp`:
- Around line 182-193: The proposal_inventory_from_counts function incorrectly
maps aggregate N3, N1, and N2 counts to disjoint simplex classes. Extend
FoliatedTriangulation4 counting, including recompute_counts_from_complex, to
retain class-resolved spacelike/timelike tetrahedra, timelike edges, and mixed
triangles, then pass those values into proposal_inventory_from_counts and
preserve the existing aggregate mappings for the remaining fields.
In `@include/Phase_analysis.hpp`:
- Around line 72-78: Implement collapse_error in analyze_finite_size_scaling
instead of leaving FiniteSizeScalingReport::collapse_error at its default zero
value. Compute an actual residual using the regression results from
fit_power_law_exponent, such as the sum of squared log-log residuals, and assign
it to the report’s collapse_error field; otherwise remove the unused field and
its exposed report value.
- Around line 416-421: Document the assignments to
diagnostics.held_out_likelihood, diagnostics.aic, and diagnostics.bic as
heuristic scores derived from the cos3 profile-shape correlation, explicitly
stating they are not formal likelihood, AIC, or BIC statistics; keep the
existing calculations and field names unchanged.
- Around line 292-346: Extract the duplicated per-profile classification from
diagnose_c_ds_finite_size and diagnose into a shared helper such as
classify_profile_shape(Profile const&, long double), preserving the
thermalization, collapse, and alternating-slice verdict behavior. Define named
constexpr thresholds for the collapse peak, alternating ratio, and cos3
correlation limits, then update both callers to reuse the helper and constants.
- Around line 103-146: Validate profile dimensions at the start of mean and
covariance before any indexed access: ensure every profile matches
profiles.front().size() in mean, and every profile matches profile_mean.size()
in covariance. Fail clearly via the project’s established throw or assertion
mechanism when a mismatch is detected, while preserving the existing empty-input
behavior.
In `@include/Simulation_output.hpp`:
- Around line 88-96: The output currently persists only the final profile via
spatial_volume_profile(); update the result-writing block to persist every
per-measurement spatial profile from result.measurements in a
spatial_volume_profiles.csv file using step,timeslice,N3 rows. Preserve the
existing final-profile output and use each measurement’s profile so external
analysis can reconstruct covariance and autocorrelation.
- Around line 111-120: The move_statistics.csv writer currently outputs the
numeric index instead of a stable move name. In the loop using
result.move_stats, include Move_catalog_4.hpp descriptors and convert each index
with as_move_4d(index), then write the corresponding descriptor name in the
first column while preserving the existing statistics columns.
- Around line 122-130: Introduce a shared stream-precision helper using
<iomanip>, <limits>, and <ostream>, and apply it immediately after opening every
numeric output stream for action_trace.csv, measurements.jsonl, covariance.csv,
effective_action.csv, manifest.json, and summary.json so long-double values
retain full precision. In the action_trace writer, validate that
result.volume_trace has at least as many entries as result.action_trace before
indexing it, and preserve safe behavior if the sizes differ.
- Around line 30-41: Update write_counts_csv_header and write_counts_csv_row to
accept std::ostream& instead of std::ofstream&, and include <ostream> in the
header. Preserve their existing CSV output and formatting so callers can provide
files or in-memory streams.
- Around line 170-200: Update the summary-generation block to pass
measured_profiles to phase::diagnose using std::move, adding the required
<utility> include. Extend summary.json to emit diagnostics.autocorrelation_time,
held_out_likelihood, aic, and bic, and include config.chain_id so results can be
attributed to their chain while preserving the existing verdict output.
- Around line 51-73: Update the manifest and summary JSON writers in
Simulation_output.hpp by adding a small JSON string-escaping helper that handles
quotes, backslashes, and control characters such as newlines. Apply it to every
interpolated string field, including the manifest values and config.chain_id,
and reuse the same helper at every string-writing site in summary.json while
leaving numeric fields unchanged.
- Around line 43-49: Update write_run_directory to validate manifest.run_id
before constructing run_dir, requiring a non-empty relative single path
component and rejecting traversal or embedded separators; throw an appropriate
exception and include the needed <stdexcept> header. Also check every
std::ofstream opened within write_run_directory and fail immediately when
opening or writing cannot proceed, so callers do not report successful output
after an I/O error; apply the same success-checking behavior to
Metropolis_4.hpp::save_checkpoint.
In `@scripts/run-cds-phase-scan.ps1`:
- Around line 37-59: Update the scan loops around the `$cdt` invocation to stop
immediately when the native command returns a non-zero `$LASTEXITCODE`,
preventing partial results from being reported. Replace the current `$BaseSeed +
$chain` calculation with a deterministic seed derived from the full parameter
point and chain, including the coupling values and target volume, so every run
has a unique seed. Apply the same parameter-point seed fix in the corresponding
shell scan script.
- Around line 32-36: Validate each coupling point in the PowerShell loop before
assigning or using its fields: after splitting $point, require at least three
components and reject malformed entries with a clear error instead of continuing
with null values. Apply the equivalent validation to the shell script’s read of
COUPLING_POINTS so missing KAPPA4 or DELTA values terminate the run rather than
being passed as empty arguments.
In `@scripts/run-cds-phase-scan.sh`:
- Around line 30-35: Disable pathname expansion in scripts/run-cds-phase-scan.sh
by adding set -f alongside the existing set -euo pipefail, preserving the
intentional unquoted word splitting in the COUPLING_POINTS and TARGET_VOLUMES
loops without allowing wildcard characters to expand into filenames.
- Around line 19-28: Update the CDT executable discovery block around CDT_EXE to
check the four candidate paths through a loop, selecting the first executable
found; if none is executable, fail immediately with a clear message matching the
PowerShell script’s “Could not find cdt executable under …” behavior before the
later scan command runs.
In `@src/cdt.cpp`:
- Around line 151-157: Validate both command-line identifiers after parsing:
reject empty, absolute, or path-separator-containing values for run_id and
chain_id before constructing output paths or manifests. Apply the validation to
the option-handling flow around the run_id and chain_id variables, preserving
the existing defaults and reporting an invalid argument clearly.
- Around line 337-352: Extend the 4D result-reporting block after the
elapsed-time/output messages to print statistics from result.move_stats and
result.measurements: total attempted, accepted, and invalid moves, the final N4,
and the measurement count. Keep the existing output-location and Standard CDT
candidate reporting unchanged, and ensure these values are shown even when
write_files is false.
- Around line 293-318: Make the 4D run control use sweep-equivalent work rather
than treating checked_passes as raw Metropolis4::run steps: scale the steps
argument by a sweep size derived from the current triangulation volume, or
consistently rename the 4D option to move attempts and update scan-script
defaults to a thermalizing value. Preserve the existing 3D pass semantics and
ensure the 4D scan configuration performs enough work relative to target volume
and thermalization.
- Around line 234-291: Move the 4D parameter validation from main into a new
runtime_config::make_4d_simulation function, reusing the shared configuration
validation for simplices, timeslices, passes, and checkpoint and throwing
invalid_argument for invalid command-line values; have main call this factory
and remain a thin driver. Ensure the returned configuration also validates the
remaining 4D parameters currently checked inline, and explicitly handle the
parsed --threads option by honoring it or reporting that it does not apply to
4D.
- Around line 60-67: Update the usage synopsis in the command-line help near the
existing options such as --volume-epsilon to include --thermalization,
--measurement-interval, --chain-id, --run-id, and --output-dir, matching the
option names and argument placeholders defined in the argument parsing section.
In `@src/CMakeLists.txt`:
- Around line 73-83: Add a follow-up CTest named cdt-d4-artifacts after cdt-d4
that uses ${CMAKE_COMMAND} -E cat to verify manifest.json, summary.json, and
checkpoint/state.txt exist, and set LABELS "integration;4d" with DEPENDS cdt-d4.
Add a cdt-d4-cleanup fixture with FIXTURES_SETUP to remove the prior output
directory before cdt-d4 runs, preventing stale artifacts from satisfying the
assertion.
In `@tests/Checkpoint_4_test.cpp`:
- Around line 9-35: Expand the checkpoint tests around
Metropolis4::load_checkpoint to assert errors for a missing state.txt, truncated
or malformed contents, and a state.txt without an rng line; these cases must not
silently return a default triangulation or preserve the unchanged RNG.
Strengthen the happy-path test by comparing resumed rng_state() and the resumed
action_trace and volume_trace against the corresponding tails of the
uninterrupted direct_result after combining partial_result with resumed_result,
while retaining the canonical_hash and step checks.
- Around line 22-25: Update the checkpoint test around partial.save_checkpoint
to use a unique temporary directory per test execution instead of the fixed
"cdtpp-checkpoint-test" path. Encapsulate directory cleanup in an RAII guard so
remove_all runs both on normal completion and when an assertion aborts the test,
including removing any existing setup directory safely.
In `@tests/Detailed_balance_test.cpp`:
- Around line 44-51: The test case should cover verifier failures in addition to
the existing success path. Extend the detailed-balance tests around
verify_detailed_balance to use a state with a move lacking a reverse transition
and assert report.errors is non-empty and report.passed is false; also verify
THREE_THREE is self-inverse, including its direction flag, and check
report.edges contains the expected move values rather than only being non-empty.
- Around line 33-41: Update the test around acceptance probability to call the
production acceptance_probability function from Detailed_balance_4.hpp instead
of reimplementing its formula locally. Validate finiteness directly on the
resulting long double value without narrowing it to double, while preserving the
existing range checks.
- Around line 21-24: Remove the tautological candidate-count assertions around
the moves::apply results, since they compare values guaranteed to match. Replace
them with an assertion that verifies the forward and reverse proposal weights
required for detailed balance, using the existing weight calculation used by the
test or moves API. Preserve the triangulation canonical_hash round-trip
assertion.
In `@tests/Foliated_triangulation_4_test.cpp`:
- Around line 30-35: Expand the test case around FoliatedTriangulation4 with
doctest SUBCASE blocks covering distinct-state canonical_hash changes after
apply_move, counts matching the move descriptor delta, applying THREE_THREE
twice returning the original state, time_reversed() matching reversed vertex
times, and from_counts_for_validation producing the expected validate() error
text. Keep the existing copy-hash assertion and use the relevant apply_move,
time_reversed, counts, and validate APIs.
- Around line 46-49: Update the inventory assertions in the test around
from_counts and seeded to compare each proposal_inventory() field against
concrete expected values rather than against the corresponding value from the
other object. Preserve coverage for spatial_tetrahedra and mixed_triangles so
regressions in proposal_inventory_from_counts are detected.
In `@tests/Metropolis_4_test.cpp`:
- Around line 23-31: Update the determinism assertions in the test’s
action_trace loop to use exact CHECK_EQ comparisons for the long-double values
instead of doctest::Approx. Extend the determinism coverage with a separate run
using a different Metropolis4Config::seed and assert that its action trace
differs from the identical-seed result, while keeping the existing same-seed
equality checks intact.
- Around line 34-52: Update the test case “Metropolis4 records invalid proposals
and accepted moves” to accumulate attempted and accepted using Int_precision,
and add assertions that aggregate and verify stat.invalid so the test covers
invalid proposals as named. Replace the non-informative CHECK_GE(accepted, 0)
with meaningful expected-count assertions, preferably exact counts for seed 7
while preserving the existing attempted-count and triangulation-validity checks.
In `@tests/Phase_analysis_test.cpp`:
- Around line 56-92: Extract the duplicated synthetic profile construction from
both TEST_CASEs into a shared helper named build_synthetic_c_ds_profiles,
preserving the existing volumes, width calculation, profile generation, and
amplitude scaling. Replace each local profiles initialization loop with a call
to this helper so both tests use the same recipe.
---
Outside diff comments:
In `@tests/Phase_analysis_test.cpp`:
- Around line 1-105: Add direct TEST_CASEs for estimate_autocorrelation_time
covering constant input, positively and negatively correlated series, and series
shorter than the three-sample minimum, asserting the documented results. Add a
to_string round-trip test that exercises every Verdict enum value and verifies
each converted string maps to the expected value or representation. Keep these
tests organized alongside the existing phase-analysis coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67087d07-bad9-413a-b2ec-c779fcbdb5b5
📒 Files selected for processing (28)
docs/4d-standard-cdt-candidate.mdinclude/Detailed_balance_4.hppinclude/Ergodic_moves_4.hppinclude/Foliated_triangulation_4.hppinclude/Metropolis_4.hppinclude/Move_catalog_4.hppinclude/Move_tracker.hppinclude/Observables_4.hppinclude/Phase_analysis.hppinclude/S4Action.hppinclude/Simulation_output.hppscripts/run-cds-phase-scan.ps1scripts/run-cds-phase-scan.shsrc/CMakeLists.txtsrc/cdt.cpptests/CMakeLists.txttests/Checkpoint_4_test.cpptests/Detailed_balance_test.cpptests/Ergodic_moves_4_test.cpptests/Foliated_triangulation_4_test.cpptests/Geometry_4_test.cpptests/Manifold_4_test.cpptests/Metropolis_4_test.cpptests/Move_tracker_test.cpptests/Observables_4_test.cpptests/Phase_analysis_test.cpptests/S4Action_test.cpptests/Time_reversal_4_test.cpp
|
Thanks for the PR, looks like there's a few issues to resolve. I'm going to push forward on modernizing and getting to a stable v1.0.0, so this doesn't keep changing out from underneath you. |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/Foliated_triangulation_4_test.cpp (1)
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the test name to match the new assertions.
The case is named "Abstract 4D canonical hash is stable for copies", but Lines 37-46 now assert
apply_movebehavior and theTWO_FOURcount delta. Split the move-delta assertions into a separateTEST_CASE, or group both properties inSUBCASEblocks so a failure identifies the broken property.♻️ Proposed structure
TEST_CASE("Abstract 4D canonical hash is stable for copies") { auto triangulation = FoliatedTriangulation4::periodic_seed(3); - auto copy = triangulation; - CHECK_EQ(copy.canonical_hash(), triangulation.canonical_hash()); - - auto moved = triangulation; + SUBCASE("A copy hashes identically") + { + auto copy = triangulation; + CHECK_EQ(copy.canonical_hash(), triangulation.canonical_hash()); + } + SUBCASE("TWO_FOUR changes the hash and applies the descriptor delta") + { + auto moved = triangulation; + // existing assertions + } }As per path instructions: "Descriptive test names are used to clearly convey the intent of each test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Foliated_triangulation_4_test.cpp` around lines 31 - 47, The test named “Abstract 4D canonical hash is stable for copies” now also covers TWO_FOUR move behavior and count deltas. Split the move/count assertions into a separate TEST_CASE or organize the existing checks into descriptive SUBCASE blocks, ensuring each test name clearly identifies canonical-hash stability versus move-delta behavior.Source: Path instructions
include/Detailed_balance_4.hpp (2)
103-129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip the transition-validation pass when enumeration is capped.
When
cap_reachedbecomes true,statesis an incomplete snapshot of the reachable graph, not a depth-truncated one. The second loop (lines 131-155) still runs over this incomplete set and reports "Reachable transition escaped enumeration depth" for every un-enumerated transition. This adds multiple misleading errors on top of the real "reached max_states" error, and obscures the actual root cause for anyone readingreport.errors.Skip the second loop, or return the report immediately, when
cap_reachedis true.🩹 Proposed fix
} } + if (cap_reached) { return report; } + for (auto const& [from_hash, from_state] : states)Also applies to: 131-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Detailed_balance_4.hpp` around lines 103 - 129, Skip the transition-validation loop after the enumeration loop when cap_reached is true, or return the report immediately at that point. Preserve the existing max_states error as the sole error for capped enumeration, and run validation only for complete, uncapped state sets.
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
boltzmann_weight.
boltzmann_weightis only declared ininclude/Detailed_balance_4.hpp; the verifier now useslog_boltzmann_weightexclusively. Drop the helper unless another public API call site is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Detailed_balance_4.hpp` around lines 66 - 71, Remove the unused boltzmann_weight helper from Detailed_balance_4.hpp, including its declaration and implementation. Keep log_boltzmann_weight and all other detailed-balance APIs unchanged.Source: Linters/SAST tools
include/Ergodic_moves_4.hpp (1)
67-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd call sites for the 4D per-move wrappers.
do_24_move,do_42_move,do_33_move,do_46_move,do_64_move,do_28_move, anddo_82_movehave no call sites andMetropolis_4.hppusesmoves::apply(...)directly. Add tests/usage for these wrappers, or remove them if they are only dead public API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Ergodic_moves_4.hpp` around lines 67 - 107, Address the unused public wrappers in Ergodic_moves_4.hpp: add tests or production call sites that exercise do_24_move, do_42_move, do_33_move, do_46_move, do_64_move, do_28_move, and do_82_move, replacing the corresponding direct moves::apply usage in Metropolis_4.hpp where appropriate; otherwise remove these wrappers and keep callers using the existing apply path.Source: Linters/SAST tools
tests/Metropolis_4_test.cpp (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the periodic checkpoint path.
Metropolis4::runnow writes a checkpoint inside the loop whencheckpoint_interval > 0andcheckpoint_directoryis set (include/Metropolis_4.hppLines 219-225). No test in this file or intests/Checkpoint_4_test.cppsetscheckpoint_intervaltogether withcheckpoint_directory, so the new branch is unexercised.include/Runtime_config.hppalways sets both for a 4D run with output enabled, so this is the production path.Add a test case that sets both fields, runs the sampler, and asserts that
state.txtexists and reports the expected step.As per path instructions, "Ensure ... Comprehensive test coverage and proper test organization."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Metropolis_4_test.cpp` around lines 46 - 52, Add a focused test for the checkpoint path in Metropolis4, setting both checkpoint_interval and checkpoint_directory on the configuration before invoking Metropolis4::run. Assert that the generated state.txt exists and records the expected checkpoint step, using the test suite’s existing temporary-directory and cleanup conventions.Source: Path instructions
♻️ Duplicate comments (3)
tests/Detailed_balance_test.cpp (2)
22-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese two assertions cannot fail; they compare a value with itself.
reverseis computed onforward->triangulation.moves::applycomputesreverse_candidateson the returned state andforward_candidateson the input state, soforward->reverse_candidatesandreverse->forward_candidatesare the same quantity, and likewise for the second line. Only the hash round-trip on lines 24-25 tests anything.Assert the proposal-weight property that detailed balance needs instead, as suggested in the earlier review.
💚 Proposed assertion change
- CHECK_EQ(forward->reverse_candidates, reverse->forward_candidates); - CHECK_EQ(forward->forward_candidates, reverse->reverse_candidates); + CHECK_GT(forward->forward_candidates, 0); + CHECK_GT(forward->reverse_candidates, 0); + CHECK_EQ(proposal_probability(triangulation, + move_tracker::MoveType4D::TWO_FOUR), + proposal_probability(reverse->triangulation, + move_tracker::MoveType4D::TWO_FOUR));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Detailed_balance_test.cpp` around lines 22 - 23, Replace the self-comparison assertions in the detailed-balance test with an assertion of the proposal-weight property required for detailed balance, using the existing forward and reverse states and the earlier review’s suggested relationship. Keep the hash round-trip assertions unchanged.Source: Path instructions
43-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerifier failure-path coverage improved, but three previously-requested cases are still missing.
The two new tests (capped enumeration, non-finite weights) add real failure-path coverage that was missing before. The three specific gaps from the earlier review remain open, though:
- No test constructs a state whose move lacks a reverse transition to exercise the "Missing reverse transition" branch directly.
- No test asserts
THREE_THREEself-inverse behavior (the direction-flag path inFoliatedTriangulation4).- Line 49 still checks only
report.edges.empty(), not thatreport.edgescontains the expectedmovevalues.Do you want draft cases for these three?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Detailed_balance_test.cpp` around lines 43 - 69, Extend the detailed-balance tests around verify_detailed_balance with three cases: construct a state whose move has no reverse transition and assert the “Missing reverse transition” failure, verify THREE_THREE self-inverse behavior including its direction flag, and strengthen the successful test to inspect report.edges for the expected move values rather than only checking non-emptiness.Source: Path instructions
include/Metropolis_4.hpp (1)
346-354: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate the RNG state before you apply it.
set_rng_state(state)runs first, and it throws throughcdt::Random::set_engine_statewhen the text is invalid. The guard on Line 351 then never executes for that case, because!file && state.empty()is only reachable when the label was read but the line is empty, whichset_engine_statealready rejects.Random::set_engine_statealso extracts directly intom_engine, so a partial state can be written before the throw.Check the parsed text first, then apply it.
🛡️ Proposed fix
require_label(file, "rng"); std::string state; std::getline(file, state); if (!state.empty() && state.front() == ' ') { state.erase(state.begin()); } - set_rng_state(state); - if (!file && state.empty()) - { - throw std::runtime_error{"Malformed checkpoint rng state."}; - } + if (state.empty()) + { + throw std::runtime_error{"Malformed checkpoint rng state."}; + } + set_rng_state(state);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Metropolis_4.hpp` around lines 346 - 354, Validate the parsed RNG state before calling set_rng_state in the checkpoint-loading flow, using the existing Random::set_engine_state validation mechanism without mutating the active engine on failure. Move the malformed-state check ahead of set_rng_state and ensure invalid or incomplete text is rejected before applying any state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Detailed_balance_4.hpp`:
- Around line 80-93: Update verify_detailed_balance to reject max_depth values
less than or equal to zero before enumeration, mirroring the existing max_states
validation. Set report.passed to false, add a descriptive validation error, and
return the report without traversing the frontier.
- Around line 156-186: Update acceptance_probability to detect non-finite
S4_action_difference or its raw acceptance ratio before applying std::min;
return or propagate NaN for invalid inputs so NaN cannot be converted to 1.0L.
Preserve normal capping for finite ratios, allowing the forward_acceptance and
reverse_acceptance checks in the detailed-balance verification flow to report
invalid transitions.
In `@include/Foliated_triangulation_4.hpp`:
- Around line 471-477: Correct the inverted m_closed_s3_slices polarity in
topology_matches_closed_s3_slices at
include/Foliated_triangulation_4.hpp:471-477 by requiring !m_closed_s3_slices ||
spatial_slices_are_connected(). Also update the guard at
include/Foliated_triangulation_4.hpp:1025-1028 to use m_closed_s3_slices &&
!simplex_neighbor_graph_connected() so disconnected states claiming closed S3
slices trigger the error.
- Around line 332-342: Update the class documentation to explicitly define the
topology contract: topology accessors such as has_closed_s3_slices(),
spatial_topology(), and validate() use declared topology for abstract count-only
states without m_simplices, while derived topology is used only when a complex
is present. Keep derived_slice_euler_characteristics() behavior unchanged.
- Around line 510-527: Update apply_spatial_profile_delta so each volume change
is assigned to the actual slice affected by the move rather than always
m_spatial_profile.front(). Propagate the target slice from the sampler or move
descriptor into this method, validate its bounds, and apply the signed delta to
that indexed profile entry while preserving existing size and underflow checks.
- Around line 490-496: The move-count update in add_count_delta must preserve
class-resolved proposal counts by applying each move’s delta instead of clearing
the resolved flag and zeroing the four counters; periodic_seed must likewise
initialize these counters for the initial state. In
include/Foliated_triangulation_4.hpp lines 490-496, update the class-resolved
counters from the move delta. In include/Move_catalog_4.hpp lines 185-203,
restrict or remove the aggregate fallback in proposal_inventory_from_counts so
callers cannot silently map counts.N3 to both tetrahedra classes.
- Around line 627-641: Update FoliatedTriangulation4::from_checkpoint_state to
synchronize m_counts with the restored simplex complex after assigning vertices
and simplices: when the restored containers are present, recompute counts from
the simplices and use those counts to rebuild proposal inventory. Preserve the
checkpoint-provided counts only when the complex is unavailable, ensuring
max_vertex_order() reflects the restored structure.
In `@include/Phase_analysis.hpp`:
- Around line 238-271: Extract the shared positive-sample filtering logic into a
helper such as positive_log_log_samples and update both fit_power_law_exponent
and log_log_residual to use it, ensuring both calculations operate on the same
sample set. Also replace the infinity fallback used by log_log_residual for
fewer than two usable samples with the project’s finite error/sentinel
representation so FiniteSizeScalingReport::collapse_error remains
JSON-serializable.
In `@include/Random.hpp`:
- Around line 146-161: Document on engine_state() and set_engine_state() that
restoring serialized state updates only m_engine, while seed() and stream()
retain their construction values; consequently, split(seed(), stream()) does not
represent the restored progressed position. Ensure engine_state() documentation
identifies the state as checkpoint data required to resume one reproducible run.
In `@include/Runtime_config.hpp`:
- Around line 286-296: The path-component validation rule is duplicated and must
be centralized. Move validate_path_component from
include/Runtime_config.hpp:286-296 into a shared cdt::utilities header, then
have the detail code call that utility; remove the duplicate in
include/Simulation_output.hpp:63-73 and call the same utility there while
preserving the existing std::invalid_argument message contract.
- Around line 158-234: Add explicit public declarations for Simulation4D’s copy
constructor, move constructor, copy assignment operator, move assignment
operator, and destructor, matching the corresponding declarations and visibility
used by sibling types Triangulation and Simulation. Keep the existing
constructor, accessors, and member storage unchanged.
In `@include/S4Action.hpp`:
- Around line 29-33: Separate proposal bookkeeping from S4Counts by introducing
an S4ClassResolvedCounts aggregate containing class_resolved_proposals’ four
counters, and store it as an optional member appended after N14. Update
proposal_inventory_from_counts and all related accesses to use optional presence
instead of a validity flag plus zeroed counters, and update
FoliatedTriangulation4::add_count_delta to clear the optional value when counts
become stale. Preserve existing aggregate initialization compatibility by
keeping the new member at the end.
In `@include/Simulation_output.hpp`:
- Around line 41-61: Update json_escape so every remaining character below 0x20
is emitted as a JSON \u00XX escape instead of being appended raw. Preserve the
existing named escapes for quotes, backslashes, and standard control characters,
and ensure the fallback produces two uppercase hexadecimal digits for the byte
value.
In `@tests/Checkpoint_4_test.cpp`:
- Around line 15-32: Update TempDirectory::~TempDirectory to use the
std::error_code overload of std::filesystem::remove_all so destruction is
non-throwing, adding the required system_error include. Delete TempDirectory’s
copy constructor and copy assignment operator to prevent multiple instances from
removing the same directory.
In `@tests/Detailed_balance_test.cpp`:
- Line 38: Update the finiteness assertion in the detailed balance test to pass
the long double probability value directly to std::isfinite, removing the
static_cast<double> narrowing conversion.
In `@tests/Foliated_triangulation_4_test.cpp`:
- Around line 96-125: Split the three independent scenarios in the test body
into separate SUBCASE blocks: timeslice clamping, aggregate proposal inventory
fallback, and class-resolved inventory override, preserving their existing
assertions. Add failure-path tests for validate() covering negative profiles,
vertex-time cache errors, disconnected connectivity, and the “Spatial slices are
not validated as connected S3 slices.” condition, including a disconnected
complex fixture that reaches the connectivity check in FoliatedTriangulation4.
In `@tests/Phase_analysis_test.cpp`:
- Around line 109-115: Update the mismatched-dimensions test around mean and
covariance so each [[nodiscard]] result is consumed in a [[maybe_unused]] const
auto variable within the CHECK_THROWS_AS expression. Preserve the existing
invalid_argument assertions and test inputs.
---
Outside diff comments:
In `@include/Detailed_balance_4.hpp`:
- Around line 103-129: Skip the transition-validation loop after the enumeration
loop when cap_reached is true, or return the report immediately at that point.
Preserve the existing max_states error as the sole error for capped enumeration,
and run validation only for complete, uncapped state sets.
- Around line 66-71: Remove the unused boltzmann_weight helper from
Detailed_balance_4.hpp, including its declaration and implementation. Keep
log_boltzmann_weight and all other detailed-balance APIs unchanged.
In `@include/Ergodic_moves_4.hpp`:
- Around line 67-107: Address the unused public wrappers in Ergodic_moves_4.hpp:
add tests or production call sites that exercise do_24_move, do_42_move,
do_33_move, do_46_move, do_64_move, do_28_move, and do_82_move, replacing the
corresponding direct moves::apply usage in Metropolis_4.hpp where appropriate;
otherwise remove these wrappers and keep callers using the existing apply path.
In `@tests/Foliated_triangulation_4_test.cpp`:
- Around line 31-47: The test named “Abstract 4D canonical hash is stable for
copies” now also covers TWO_FOUR move behavior and count deltas. Split the
move/count assertions into a separate TEST_CASE or organize the existing checks
into descriptive SUBCASE blocks, ensuring each test name clearly identifies
canonical-hash stability versus move-delta behavior.
In `@tests/Metropolis_4_test.cpp`:
- Around line 46-52: Add a focused test for the checkpoint path in Metropolis4,
setting both checkpoint_interval and checkpoint_directory on the configuration
before invoking Metropolis4::run. Assert that the generated state.txt exists and
records the expected checkpoint step, using the test suite’s existing
temporary-directory and cleanup conventions.
---
Duplicate comments:
In `@include/Metropolis_4.hpp`:
- Around line 346-354: Validate the parsed RNG state before calling
set_rng_state in the checkpoint-loading flow, using the existing
Random::set_engine_state validation mechanism without mutating the active engine
on failure. Move the malformed-state check ahead of set_rng_state and ensure
invalid or incomplete text is rejected before applying any state.
In `@tests/Detailed_balance_test.cpp`:
- Around line 22-23: Replace the self-comparison assertions in the
detailed-balance test with an assertion of the proposal-weight property required
for detailed balance, using the existing forward and reverse states and the
earlier review’s suggested relationship. Keep the hash round-trip assertions
unchanged.
- Around line 43-69: Extend the detailed-balance tests around
verify_detailed_balance with three cases: construct a state whose move has no
reverse transition and assert the “Missing reverse transition” failure, verify
THREE_THREE self-inverse behavior including its direction flag, and strengthen
the successful test to inspect report.edges for the expected move values rather
than only checking non-emptiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e124143c-bf57-40c7-ba08-4d52066f5dff
📒 Files selected for processing (19)
include/Detailed_balance_4.hppinclude/Ergodic_moves_4.hppinclude/Foliated_triangulation_4.hppinclude/Metropolis_4.hppinclude/Move_catalog_4.hppinclude/Phase_analysis.hppinclude/Random.hppinclude/Runtime_config.hppinclude/S4Action.hppinclude/Simulation_output.hppscripts/run-cds-phase-scan.ps1scripts/run-cds-phase-scan.shsrc/CMakeLists.txtsrc/cdt.cpptests/Checkpoint_4_test.cpptests/Detailed_balance_test.cpptests/Foliated_triangulation_4_test.cpptests/Metropolis_4_test.cpptests/Phase_analysis_test.cpp
|
@acgetchell I have updated pr |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
include/Foliated_triangulation_4.hpp (1)
982-999: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject simplex vertices that are absent from
m_vertices.
vertex_time()maps an unknown ID to-1. Periodic time arithmetic can then classify a simplex with dangling vertex IDs as causal. For example, four unknown IDs and one vertex at time zero can pass asFOUR_ONE.Validate every
simplex.verticesID againstm_vertex_timesbefore causal classification. Report the missing reference and skip further validation of that simplex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 982 - 999, Before calling classify_simplex in the m_simplices validation loop, verify every ID in simplex.vertices exists in m_vertex_times. If any vertex is missing, report the missing vertex reference and skip all remaining validation for that simplex, including causal-type classification.include/Phase_analysis.hpp (1)
205-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-finite sample coordinates.
+infinitypasses the positivity check.std::log(+infinity)can causefit_power_law_exponentto returnNaN.analyze_finite_size_scalingthen exposes non-finite exponents even thoughcollapse_erroruses a finite sentinel.Require both coordinates to be finite before converting them. Add a regression test with an infinite volume or measurement.
Proposed fix
- if (sample.first > 0.0L && sample.second > 0.0L) + if (std::isfinite(sample.first) && std::isfinite(sample.second) && + sample.first > 0.0L && sample.second > 0.0L)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Phase_analysis.hpp` around lines 205 - 245, Update positive_log_log_samples to require both sample coordinates to be finite and greater than zero before applying std::log, using the existing standard finite-value check. Add a regression test covering an infinite volume or measurement and verify analyze_finite_size_scaling does not expose a non-finite exponent.include/Simulation_output.hpp (1)
90-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFlush and close each output file before reporting success.
ensure_wrotechecks only the buffered stream state. A disk-full or delayed I/O failure can occur during flush or close after this function returns. Acceptstd::ofstream&, then flush, close, and check the stream before returning success.As per path instructions, “Focus on design patterns, interfaces, and encapsulation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Simulation_output.hpp` around lines 90 - 97, Update ensure_wrote to accept an std::ofstream& instead of a generic stream, then flush and close the output file before validating success. Check the stream state after these operations and throw the existing path-specific runtime error on failure, preserving the current success behavior otherwise.Source: Path instructions
include/Random.hpp (1)
159-166: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject trailing data in serialized engine state.
input >> m_enginecan succeed while leaving trailing non-whitespace data unread. A malformed checkpoint RNG field can therefore be accepted. Consume whitespace after extraction and requireinput.eof()before storing the state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Random.hpp` around lines 159 - 166, Update set_engine_state so it validates the entire serialized state: after extracting m_engine, consume trailing whitespace and require input.eof() before accepting the state; throw the existing invalid-state error when non-whitespace trailing data remains.Source: Path instructions
♻️ Duplicate comments (1)
include/Metropolis_4.hpp (1)
298-299: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the prior checkpoint until replacement succeeds.
Line 298 deletes
state.txtbefore Line 299 replaces it. Ifrenamefails, the sampler loses the last valid checkpoint. Use an atomic replace operation that replaces the destination directly on each supported platform. The existing platform-aware replacement policy ininclude/Utilities.hppprovides this behavior.As per path instructions, “Focus on design patterns, interfaces, and encapsulation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Metropolis_4.hpp` around lines 298 - 299, Update the checkpoint replacement logic around std::filesystem::remove and std::filesystem::rename to use the existing platform-aware atomic replacement policy from Utilities.hpp, replacing target directly without deleting the prior checkpoint first. Preserve the last valid state.txt until the replacement succeeds.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Detailed_balance_4.hpp`:
- Around line 73-76: Apply the repository’s clang-format style to the
conditional return expression in the probability calculation, specifically the
return statement following std::exp(log_ratio). Preserve the existing behavior
and values while correcting only the formatting of the std::isfinite conditional
expression.
In `@tests/Simulation_output_test.cpp`:
- Around line 9-17: Extend the doctest coverage beyond json_escape with
integration cases for write_run_directory: write a deterministic
Metropolis4Result and verify each generated output file, assert invalid run_id
values are rejected, and assert mismatched action and volume traces are
rejected. Organize these as focused doctest cases covering the public output
contract.
---
Outside diff comments:
In `@include/Foliated_triangulation_4.hpp`:
- Around line 982-999: Before calling classify_simplex in the m_simplices
validation loop, verify every ID in simplex.vertices exists in m_vertex_times.
If any vertex is missing, report the missing vertex reference and skip all
remaining validation for that simplex, including causal-type classification.
In `@include/Phase_analysis.hpp`:
- Around line 205-245: Update positive_log_log_samples to require both sample
coordinates to be finite and greater than zero before applying std::log, using
the existing standard finite-value check. Add a regression test covering an
infinite volume or measurement and verify analyze_finite_size_scaling does not
expose a non-finite exponent.
In `@include/Random.hpp`:
- Around line 159-166: Update set_engine_state so it validates the entire
serialized state: after extracting m_engine, consume trailing whitespace and
require input.eof() before accepting the state; throw the existing invalid-state
error when non-whitespace trailing data remains.
In `@include/Simulation_output.hpp`:
- Around line 90-97: Update ensure_wrote to accept an std::ofstream& instead of
a generic stream, then flush and close the output file before validating
success. Check the stream state after these operations and throw the existing
path-specific runtime error on failure, preserving the current success behavior
otherwise.
---
Duplicate comments:
In `@include/Metropolis_4.hpp`:
- Around line 298-299: Update the checkpoint replacement logic around
std::filesystem::remove and std::filesystem::rename to use the existing
platform-aware atomic replacement policy from Utilities.hpp, replacing target
directly without deleting the prior checkpoint first. Preserve the last valid
state.txt until the replacement succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 573afc13-5bc7-471a-8169-2670a248851d
📒 Files selected for processing (16)
include/Detailed_balance_4.hppinclude/Foliated_triangulation_4.hppinclude/Metropolis_4.hppinclude/Move_catalog_4.hppinclude/Phase_analysis.hppinclude/Random.hppinclude/Runtime_config.hppinclude/S4Action.hppinclude/Simulation_output.hppinclude/Utilities.hpptests/CMakeLists.txttests/Checkpoint_4_test.cpptests/Detailed_balance_test.cpptests/Foliated_triangulation_4_test.cpptests/Phase_analysis_test.cpptests/Simulation_output_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/Foliated_triangulation_4.hpp (1)
727-740: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate profile-centering logic between this file and
Phase_analysis.hpp.
centered_spatial_volume_profile()reimplements the same peak-finding andstd::rotatecentering asPhase_analysis.hpp::centered(). Two independent implementations of the same numeric transform can drift apart silently if one is updated (for example, a tie-breaking rule formax_elementor rounding change) without the other. Extract a single sharedcentered(Profile)helper and have both call sites use it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 727 - 740, Extract the shared peak-finding and rotation logic from centered_spatial_volume_profile() and Phase_analysis.hpp::centered() into one reusable centered(Profile) helper. Update both call sites to delegate to that helper, preserving the existing empty-profile behavior and centering semantics while ensuring tie-breaking and offset calculations have a single implementation.include/Metropolis_4.hpp (1)
276-282: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a sentinel that cannot coincide with a stored simplex id.
SimplexIdisstd::uint64_t, and simplex ids are assigned from non-negative list indices, soSimplexId{0}can represent valid real simplex ids. The currentvalue_or(SimplexId{0})/neighbor_id == 0 ? std::optional<SimplexId>{}conversion can turn a real adjacency into “no neighbor” on checkpoint serialization/deserialization. Use a separate per-neighbor marker or a distinct sentinel outside the valid simplex id range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Metropolis_4.hpp` around lines 276 - 282, Update the simplex checkpoint serialization and deserialization around simplex.neighbors and neighbor_id so missing neighbors use a marker that cannot equal any valid non-negative SimplexId. Replace the current SimplexId{0} fallback and corresponding zero-to-nullopt conversion with a distinct sentinel or explicit per-neighbor presence marker, preserving valid adjacency to simplex ID 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Foliated_triangulation_4.hpp`:
- Around line 905-914: Update validate() so standard_cdt_candidate becomes false
whenever the periodic-time or closed-S3-slice checks fail, preventing invalid
reports from exposing a true standard-CDT status. Preserve the existing
report.valid() composition used by summary and CLI output, and apply the change
in the branches that emit the Standard CDT validation errors.
---
Outside diff comments:
In `@include/Foliated_triangulation_4.hpp`:
- Around line 727-740: Extract the shared peak-finding and rotation logic from
centered_spatial_volume_profile() and Phase_analysis.hpp::centered() into one
reusable centered(Profile) helper. Update both call sites to delegate to that
helper, preserving the existing empty-profile behavior and centering semantics
while ensuring tie-breaking and offset calculations have a single
implementation.
In `@include/Metropolis_4.hpp`:
- Around line 276-282: Update the simplex checkpoint serialization and
deserialization around simplex.neighbors and neighbor_id so missing neighbors
use a marker that cannot equal any valid non-negative SimplexId. Replace the
current SimplexId{0} fallback and corresponding zero-to-nullopt conversion with
a distinct sentinel or explicit per-neighbor presence marker, preserving valid
adjacency to simplex ID 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95b79052-fc99-48ef-8200-a02df531edaa
📒 Files selected for processing (11)
include/Detailed_balance_4.hppinclude/Foliated_triangulation_4.hppinclude/Metropolis_4.hppinclude/Phase_analysis.hppinclude/Random.hppinclude/Simulation_output.hpptests/Foliated_triangulation_4_test.cpptests/Metropolis_4_test.cpptests/Phase_analysis_test.cpptests/Random_test.cpptests/Simulation_output_test.cpp
validate() set standard_cdt_candidate unconditionally and never cleared it, so a report failing the standard-CDT structural checks still exposed standard_cdt_candidate == true to callers reading the field directly. Clear the flag in the four branches that emit standard-CDT structural errors: non-periodic time, slices not marked closed S3, slices not validated as connected S3, and a slice whose Euler characteristic is not S3's. The report.valid() && report.standard_cdt_candidate composition used by the CLI summary and JSON output is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/Utilities.hpp (3)
100-111: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject Windows root-names at the point of path composition.
validate_path_componentdoes not reject drive root-names such as"C:". This can pass, butoutput_dir / run_id / "..."still composes the final path. On Windows, an appended path that has a root name replaces the preceding path, so"C:"can discardoutput_dirand redirect checkpoint/output writes to the wrong absolute location. Reject root-names when composingrun_idinto the output/checkpoint paths, or makevalidate_path_componentvalidate every appended component used inoperator/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Utilities.hpp` around lines 100 - 111, Update validate_path_component to reject filesystem root names, including Windows drive-qualified values such as "C:", before any component is appended with operator/. Ensure the validation covers run_id and every path component used in output/checkpoint path composition, while preserving the existing rejection of empty, dot, separator-containing, and absolute values.
248-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving the graph-canonicalization algorithm out of Utilities.hpp.
Lines 248-425 add a self-contained, ~180-line individualization-refinement canonical-labeling algorithm (
canonical_colors,refine_incidence_colors,incidence_records_for_coloring,canonical_incidence_search,canonical_bipartite_incidence_records).Utilities.hppotherwise holds broad, unrelated helpers (filenames, RNG, logging, persistence I/O). Bundling a distinct graph-theory algorithm into this file reduces cohesion and couples every translation unit that includesUtilities.hppto this additional complexity.Extracting this block into its own header (e.g.,
include/Canonical_incidence.hpp) would letUtilities.hppinclude it only where needed and would make the algorithm's interface and invariants easier to review and test in isolation.As per path instructions for
include/**/*.hppto focus on design patterns, interfaces, and encapsulation, splitting this cohesive algorithm into a dedicated header would improve module boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Utilities.hpp` around lines 248 - 425, Extract the graph-canonicalization block—canonical_colors, consume_canonical_incidence_work, refine_incidence_colors, incidence_records_for_coloring, canonical_incidence_search, and canonical_bipartite_incidence_records—from Utilities.hpp into a dedicated header such as Canonical_incidence.hpp. Preserve their interfaces, invariants, behavior, and required includes, then include the new header from Utilities.hpp only where existing callers require canonical_bipartite_incidence_records.Source: Path instructions
267-383: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the canonicalization work or route budget exhaustion back to writes.
canonical_incidence_searchbranches on every vertex in the remaining ambiguous color class, so symmetric coincident-vertex clusters can consume the fixedCANONICAL_INCIDENCE_WORK_BUDGET. When that limit is reached,canonical_bipartite_incidence_recordsthrows, which propagates throughincidence_topology_recordsandcanonical_topology_fingerprintwhilewrite_payloadcomputes payload metadata. Catch the budget-exhaustion case in metadata derivation or scale the budget with the graph size so checkpoint/writes fail intentionally before the file is published rather than mid-write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Utilities.hpp` around lines 267 - 383, Handle canonicalization budget exhaustion in the metadata derivation path used by write_payload, around canonical_bipartite_incidence_records and its incidence_topology_records/canonical_topology_fingerprint callers. Detect only the canonical incidence work-budget failure and propagate it as an intentional write/checkpoint failure before publishing the file, while preserving propagation of unrelated errors and avoiding mid-write partial publication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@include/Utilities.hpp`:
- Around line 100-111: Update validate_path_component to reject filesystem root
names, including Windows drive-qualified values such as "C:", before any
component is appended with operator/. Ensure the validation covers run_id and
every path component used in output/checkpoint path composition, while
preserving the existing rejection of empty, dot, separator-containing, and
absolute values.
- Around line 248-425: Extract the graph-canonicalization
block—canonical_colors, consume_canonical_incidence_work,
refine_incidence_colors, incidence_records_for_coloring,
canonical_incidence_search, and canonical_bipartite_incidence_records—from
Utilities.hpp into a dedicated header such as Canonical_incidence.hpp. Preserve
their interfaces, invariants, behavior, and required includes, then include the
new header from Utilities.hpp only where existing callers require
canonical_bipartite_incidence_records.
- Around line 267-383: Handle canonicalization budget exhaustion in the metadata
derivation path used by write_payload, around
canonical_bipartite_incidence_records and its
incidence_topology_records/canonical_topology_fingerprint callers. Detect only
the canonical incidence work-budget failure and propagate it as an intentional
write/checkpoint failure before publishing the file, while preserving
propagation of unrelated errors and avoiding mid-write partial publication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98daf8d7-86bf-4d92-bc68-469aca5ba719
📒 Files selected for processing (2)
include/Foliated_triangulation_4.hppinclude/Utilities.hpp
validate_path_component rejected empty, dot, separator-bearing, and absolute values, but a drive-qualified value such as "C:" is none of those on Windows: it has a root name without a root directory, so is_absolute() is false and it contains no separator. operator/ discards its left operand whenever the right operand carries a root name, so "output" / "C:" collapses to "C:". A run_id or chain_id of "C:" therefore escaped the configured output root when composing run and checkpoint directories. Reject root names and root directories as well. Every composed component routes through this function, so run_id and chain_id are both covered at their existing call sites in Runtime_config.hpp and Simulation_output.hpp. POSIX is unaffected: "C:" has no root name there and stays a valid component, so the added test is guarded on _WIN32. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@acgetchell I have updated pr again and addressed comments from CodeRabbit. Would you run the workflow again to double-check the issues? |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
include/Foliated_triangulation_4.hpp (5)
379-385: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject empty spatial slices as invalid S3 slices.
When a concrete complex has no spacelike facets in a slice, this loop skips it.
derived_slice_euler_characteristics()returns0for the same empty slice, sotopology_matches_closed_s3_slices()can accept a state with a missing slice. Require every slice to contain at least one spacelike facet before reporting a standard-CDT candidate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 379 - 385, Update spatial_slices_are_connected() to return false whenever any concrete slice has no spacelike facets, rather than continuing past empty facets. Preserve the existing m_simplices.empty() behavior and the current connectivity checks for non-empty slices so topology_matches_closed_s3_slices() cannot accept missing spatial slices.
462-468: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not treat connectivity and Euler characteristic as proof of S3 topology.
This predicate accepts any connected slice with Euler characteristic
0. A 3-torus,S2 x S1, or a non-manifold complex can satisfy those checks.spatial_topology()can therefore return"S3"for a non-S3 concrete complex. Implement an S3-specific validation, or expose this result as a heuristic instead of a validated topology.Also applies to: 917-922
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 462 - 468, The topology_matches_closed_s3_slices() predicate must not report validated S3 topology from only Euler characteristic and connectivity. Replace these checks with an S3-specific topology validation for every slice, and update the corresponding spatial_topology() handling so non-S3 complexes cannot be labeled "S3"; if validation is unavailable, expose the result as a heuristic instead.
615-631: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSynchronize all derived fields when restoring a concrete checkpoint.
from_checkpoint_state()recomputesm_countswhen either container is non-empty, but it leavesm_spatial_profilefrom the caller. A stale profile can drive observables andcanonical_hash()even though counts come from the restored simplices. The||condition also accepts a vertex-only or simplex-only payload, which later helpers can treat as an abstract state. Require both containers together, reject partial payloads, and recompute or verify the profile before rebuilding the inventory.As per path instructions, focus on design patterns, interfaces, and encapsulation for this public factory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 615 - 631, The public factory from_checkpoint_state must reject partial checkpoint payloads and synchronize all derived state: require m_vertices and m_simplices to be either both empty or both populated, recompute m_counts only for a complete concrete complex, and recompute or validate m_spatial_profile from the restored complex before proposal_inventory_from_counts. Preserve abstract empty-state restoration while preventing stale profile data from reaching observables or canonical_hash().Source: Path instructions
943-955: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate vertex domain and incidence, not only cache consistency.
m_vertex_timescan faithfully cache an invalid time. A stored time of-1can also be treated as adjacent to time0by the periodic modulo logic. An extra vertex that no simplex references also passes the current checks, whilerecompute_counts_from_complex()counts it inN0. Reject times outside[0, m_timeslices)and require every concrete vertex to be referenced by at least one simplex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 943 - 955, Extend the validation logic around the vertex-time cache checks to reject any vertex with time outside [0, m_timeslices), and verify that every concrete vertex in m_vertices is referenced by at least one simplex. Preserve the existing cache-size and cache-consistency checks, and report validation errors for invalid times or unreferenced vertices so the complex’s vertex domain and incidence match recompute_counts_from_complex().
968-1000: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate that each neighbor represents the referenced facet.
The current check confirms only reciprocal simplex IDs. A malformed complex can point
neighbors[index]to any reciprocal simplex while the opposite facet is shared by a different simplex. Facet incidence and graph connectivity can still pass. Comparefacet_vertices(simplex, index)with the neighbor's facets, and require the reciprocal edge to represent that same facet.Also applies to: 1001-1022
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Foliated_triangulation_4.hpp` around lines 968 - 1000, Update neighbor validation for each simplex and neighbor index to verify that the referenced neighbor shares the exact facet returned by facet_vertices(simplex, index), not merely a reciprocal simplex ID. Compare that facet against the neighbor’s corresponding facets and require the reciprocal edge to identify the same facet, while preserving the existing reciprocal-ID, facet-incidence, and connectivity checks.include/Utilities.hpp (1)
168-168: 🗄️ Data Integrity & Integration | 🔵 TrivialDocument the v1 causal-info compatibility boundary explicitly.
read_causal_infoonly acceptsCAUSAL_INFO_HEADER, and the reproducibility docs statecdt-plusplus-causal-info-v1payloads are rejected so existing triangulations remain topology-readable. If older v1 payloads must continue to be loadable, support them here; otherwise keep the docs’ “legacy payloads rejected” wording clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/Utilities.hpp` at line 168, Update the causal-info compatibility handling around read_causal_info and the cdt-plusplus-causal-info-v2 identifier: either add explicit support for loading cdt-plusplus-causal-info-v1 payloads, or clearly document and preserve their rejection while keeping existing triangulations topology-readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@include/Foliated_triangulation_4.hpp`:
- Around line 379-385: Update spatial_slices_are_connected() to return false
whenever any concrete slice has no spacelike facets, rather than continuing past
empty facets. Preserve the existing m_simplices.empty() behavior and the current
connectivity checks for non-empty slices so topology_matches_closed_s3_slices()
cannot accept missing spatial slices.
- Around line 462-468: The topology_matches_closed_s3_slices() predicate must
not report validated S3 topology from only Euler characteristic and
connectivity. Replace these checks with an S3-specific topology validation for
every slice, and update the corresponding spatial_topology() handling so non-S3
complexes cannot be labeled "S3"; if validation is unavailable, expose the
result as a heuristic instead.
- Around line 615-631: The public factory from_checkpoint_state must reject
partial checkpoint payloads and synchronize all derived state: require
m_vertices and m_simplices to be either both empty or both populated, recompute
m_counts only for a complete concrete complex, and recompute or validate
m_spatial_profile from the restored complex before
proposal_inventory_from_counts. Preserve abstract empty-state restoration while
preventing stale profile data from reaching observables or canonical_hash().
- Around line 943-955: Extend the validation logic around the vertex-time cache
checks to reject any vertex with time outside [0, m_timeslices), and verify that
every concrete vertex in m_vertices is referenced by at least one simplex.
Preserve the existing cache-size and cache-consistency checks, and report
validation errors for invalid times or unreferenced vertices so the complex’s
vertex domain and incidence match recompute_counts_from_complex().
- Around line 968-1000: Update neighbor validation for each simplex and neighbor
index to verify that the referenced neighbor shares the exact facet returned by
facet_vertices(simplex, index), not merely a reciprocal simplex ID. Compare that
facet against the neighbor’s corresponding facets and require the reciprocal
edge to identify the same facet, while preserving the existing reciprocal-ID,
facet-incidence, and connectivity checks.
In `@include/Utilities.hpp`:
- Line 168: Update the causal-info compatibility handling around
read_causal_info and the cdt-plusplus-causal-info-v2 identifier: either add
explicit support for loading cdt-plusplus-causal-info-v1 payloads, or clearly
document and preserve their rejection while keeping existing triangulations
topology-readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 958f0a10-87a9-419b-af60-9a52e3bc3e88
📒 Files selected for processing (3)
include/Foliated_triangulation_4.hppinclude/Utilities.hpptests/Simulation_output_test.cpp
acgetchell
left a comment
acgetchell
left a comment
There was a problem hiding this comment.
Thank you for the substantial work represented by this PR. After reviewing the implementation and its scientific contracts, I cannot approve it as 3+1D CDT support.
The central blocker is that the program does not evolve a four-dimensional causal triangulation. periodic_seed() constructs an initial complex but then clears its vertices, simplices, and time assignments. apply_move() subsequently updates only aggregate simplex counts and a spatial-volume profile; it does not select a legal local subcomplex or modify simplicial incidence data.
This causes several fundamental problems:
- Move applicability and candidate multiplicities are inferred from global counts such as
N1,N2, andN3, rather than by enumerating legal local move sites. - The proposal process does not select the local candidates assumed by the forward/reverse proposal ratio. Consequently, the detailed-balance calculation does not describe the Markov chain actually being executed.
- Some move deltas are combinatorially invalid. In particular, the declared 2→8 delta gives
[
\Delta\chi=1-6+10-10+6=1,
]
although a topology-preserving local move must have (\Delta\chi=0). - The topology is reported as (S^3\times S^1) using flags and aggregate checks after the geometry has been discarded. It is not established from a valid triangulation.
- The phase classifications are heuristic profile thresholds rather than validated CDT phase diagnostics.
- The reported “effective action” is formed by reciprocating individual covariance diagonal entries, not by computing the corrected inverse covariance matrix. The reported likelihood, AIC, and BIC values are likewise heuristic transformations of a profile correlation rather than those statistics.
The current tests do not provide independent scientific validation. Many compare the implementation against the same hard-coded move tables or synthetic functions used by the production code. The test target also currently fails to compile because counts is redeclared in Foliated_triangulation_4_test.cpp.
The Regge-action formula, RNG support, configuration handling, and portions of the output infrastructure may be reusable. However, the scientific core requires replacement: the triangulation representation, periodic seed, local moves, candidate enumeration, proposal kernel, detailed balance, topology validation, observables, and phase analysis.
Therefore, this PR cannot be merged as 3+1D CDT support. The problem is not limited to implementation defects or insufficient tests: its central computational model is different from the model this project is intended to simulate. Because the tests and surrounding interfaces encode the same count-space abstraction, this cannot reasonably be corrected through incremental changes to this PR.
I recommend closing this PR and superseding it with a new implementation developed incrementally from independently validated foundations:
- A persistent causal four-dimensional simplicial complex and verified (S^3\times S^1) seed.
- One geometrically implemented and independently tested move/inverse pair at a time.
- Candidate enumeration and a proposal kernel with demonstrated detailed balance.
- Geometry-derived observables.
- Phase classification and effective-action reconstruction only after validating the underlying Markov chain.
Some non-scientific infrastructure may be suitable for extraction into separate, narrowly scoped PRs, but the present scientific implementation should not be retained or represented as 3+1D CDT.
This is a scientific-correctness blocker, not a request for ordinary cleanup or additional test coverage.
|
@acgetchell Thank you for the response. It is exactly what I needed. Also, I will reopen the PR as soon as I address the highlighted issues. (I need 3d+1 CDT for my research, and the only way to get it is not just to write it but also to get comments/approve from the original author) |
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests