Reapply 3+1D CDT support - #138
Open
rakovpublic wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
WalkthroughThe PR adds a complete 4D CDT stack. It covers 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 Metropolis4
participant FoliatedTriangulation4
participant SimulationOutput
CLI->>Metropolis4: configure and run 4D simulation
Metropolis4->>FoliatedTriangulation4: propose and validate moves
FoliatedTriangulation4-->>Metropolis4: return updated state and measurements
Metropolis4-->>CLI: return simulation result
CLI->>SimulationOutput: write_run_directory
SimulationOutput-->>CLI: write structured run files
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
Owner
|
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests