Repository navigation
chore: consolidate develop into main - #100
Conversation
Replace foliated_triangulations::vertices_from_cell, which only works on one cell, with manifolds::get_vertices_from_cells, which works on a container of cells. Move it to the foliated_triangulations namespace.
Nothing major, unless it trips up clang-format on GitHub Actions. Also update to vcpkgGitCommitId: 9ff4659a075d5f4f30aaca9f7c4e7f059ecc1d6d
For whatever reason, the same .clang-format produces different results on clang-format-13 vs clang-format-14. So, update to use clang-format-14 on GitHub Actions.
The bistellar_flip_really function completes the (4,4) move. Of course, it has bugs that need to be fixed, namely that it apparently leaves the Triangulation Data Structure in an invalid state, thus failing all of the checks against is_valid() which eventually leads to a std::terminate. So, this will need to be fixed. Also, update vcpkgGitCommitId: 89295c9fe22e97ca9e380a6c771ccf2ff09dca95, which incorporates the fixed version of date in vcpkg which fixes the use-of-uninitialized-value result from MSAN.
Update vcpkgGitCommitId: 5d66a98f64188bbd806b8e62e66c097d90eb3224
Update vcpkgGitCommitId: c37cc7836a0e1cfd55747be8ec472eafa8055276
While debugging bistellar_flip, make sure do_44_move returns the manifold. Fix input parameter on do_44_move to be a const reference to the manifold. Update to Boost 1.79 and qt6.3 via vcpkgGitCommitId: ecd55433fd37129109f663548f3b11204e35e630 Also proposed a pull request harupy/find-trailing-whitespace#18 to only check whitespace on changed files.
Also update README.md.
Working through the minimal example in bistellar.cpp revealed that any edge was selected due to infinite incident cells being counted. So, add a check to not count infinite cells.
The source file bistellar.cpp now contains a working minimal example of a bistellar flip. Also added a CTest to check for a successful run.
Fix documentation links and update the bistellar flip example.
Add find_vertex and find_cell helper functions to FoliatedTriangulation. These are wrappers for CGAL::Triangulation_3::is_vertex and ::is_cell, which are boolean functions with out parameters. The plan is to recast these as free functions taking an Delaunay triangulation parameter, so that they can be used in the bistellar.cpp example. That way, if they are accepted into Triangulation_3 then bistellar_flip_really becomes easy to write in terms of internal Triangulation_3 functions. Also update GitHub Actions to vcpkgGitCommitId: a106de33bbee694e3be6243718aa2a549a692832. Change README to reflect that vcpkg manifest mode is now the default, so vcpkg install --feature-flags=manifests is no longer necessary.
Yes, unified call syntax here would be nice. All tests pass. Note that hilarity (i.e. bugs) ensues if foliated_triangulations::collect_edges takes a Delaunay triangulation by value instead of constant reference. Update to vcpkgGitCommitId: 47cbed88514c0e48fdb72ddc29545f35136f5461
Catch2 has a number of issues with ASAN, compile times, etc. doctest is much faster and simpler. There are a couple of tests that need to be rewritten, and have been commented out.
Update vcpkgGitCommitId: 2e781b513f486618909d9f78185c0c7ebb54624c
Mostly to try to see if we can get less output from ASAN.
Update vcpkgGitCommitId: 840f701d83d5019aa5033c9d9d08a4cc0d0ebdce which should fix spdlog.
Also take advantage of std::numbers in C++20.
Should rename functions to be consistent.
Geometry3 -> Geometry_3 FoliatedTriangulation3 -> Foliated_Triangulation_3 etc. New function incident_cells_from_edge to encapsulate all the logic behind finding locations for bistellar flips based on the number of incident cells (ie. == 4). Updated to vcpkgGitCommitId: c379c64ff9ec1b8c6100ebff8e81ecdcb49b330c
Only need a reference to the triangulation, the edge to be flipped, and the top and bottom vertices. Update vcpkgGitCommitId: 1323f4b83d6a91e415e9ce06a45c9d0a6473aaf2 The function still needs to be debugged, of course.
Turn on doctest fast assertions. Fix references to Catch in documentation and CI. Use doctest test suites. Update CI to use vcpkgGitCommitId: 9e8da9bd8aa16fb268ac68f731c44fdef5fec1ab
PVS Studio identified possible use of an uninitialized value obtained by a function returning a std::optional<T>.
# ---------------------------------------------------------- # Header - (type): Brief description # ---------------------------------------------------------- # * feature A new feature # * fix A bug fix # * docs Changes to documentation only # * style Style/format changes (whitespace, etc.) # * refactor Changes not related to a bug or feature # * performance Changes that affects performance # * test Changes that add/modify/correct tests # * build Changes to build system (configs, etc.) # * ci Changes to CI pipeline/workflow # ---------------------------------------------------------- # ---------------------------------------------------------- # Body - More detailed description, if necessary # ---------------------------------------------------------- # * Motivation behind changes, more detail into how # functionality might be affected, etc. # ---------------------------------------------------------- # ---------------------------------------------------------- # Footer - Associated issues, PRs, etc. # ---------------------------------------------------------- # * Ex: Resolves Issue #207, see PR #15, etc. # ----------------------------------------------------------
fmt::print doesn't like CGAL point << output anymore, so wrap it with a point_to_str that converts it. Fix Metropolis to use SystemError.what() which is a const string for reporting via spdlog::trace which uses fmt which requires const string arguments. Similar conversions using Gmpzf.to_double to make spdlg::debug happy. Update vcpkgGitCommitId: 23cc58477e200bb54c293ad76f3ce438dbabc86c
Nice catch via sonarcloud. Also remove unused includes.
Found a few more places to fix fmt. Simplified try_32_move. Removed diagnostic code that was throwing exceptions. That info gets printed out before the runs already. Updated vcpkgGitCommitId: 5fb8f6c24ec13a36db460a555b24b20be03bd73c
Except the usual transparent functors 'less<>' issue which is unfixable given docopt.cpp's current implementation.
Consistent formatting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- enforce causal 2-to-3 proposals and implement failure-atomic 4-to-4 moves with checked CGAL primitives - preserve manifold settings, validate runtime bounds, and prevent move counters from carrying across runs - make sanitizer, CodeQL, Codecov, and cross-platform CI failures actionable - add locked Python 3.12 quality tooling and retire PVS-Studio
Correct the immutable action pin used to install Just and align the reported C++ files with clang-format 18.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Justfile (1)
164-180: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftCheck committed changes, not only the working tree.
Line 168 compares the worktree with
HEAD; in a clean checkout, includingjust ci, that diff is empty. The formatter gate then skips every tracked C/C++ change. Diff against an explicit PR/base revision (with the base fetched) or validate tracked files directly.🤖 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 `@Justfile` around lines 164 - 180, The _format-check recipe currently compares HEAD with its worktree, so committed changes are skipped in clean checkouts and CI. Update its tracked-file validation to compare against the explicit PR/base revision, ensuring that base revision is available, or otherwise format-check all tracked C/C++ files directly; preserve the existing untracked-file validation.include/Metropolis.hpp (1)
234-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the
(6,2)action delta.A
(6,2)move removes four combined(3,1)/(1,3)cells and no(2,2)cells, but this calculates the opposite cell-type delta.A2therefore uses the wrong acceptance probability.Proposed fix
case move_tracker::move_type::SIX_TWO: newS3Action = - S3_bulk_action(m_geometry.N1_TL - 2, m_geometry.N3_31_13, - m_geometry.N3_22 - 4, m_Alpha, m_K, m_Lambda); + S3_bulk_action(m_geometry.N1_TL - 2, + m_geometry.N3_31_13 - 4, + m_geometry.N3_22, m_Alpha, m_K, m_Lambda);🤖 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.hpp` around lines 234 - 239, Update the SIX_TWO branch in the move_tracker action calculation to reflect removal of four combined (3,1)/(1,3) cells and no (2,2) cells: decrement the N3_31_13 argument by 4 while leaving N3_22 unchanged, and preserve the existing N1_TL decrement.include/Move_always.hpp (1)
112-120: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not expose silent 4D no-op strategies. Both generic strategy implementations only execute 3D moves, despite the exported 4D aliases. A 4D run either performs no moves or applies 3D acceptance logic.
include/Move_always.hpp#L112-L120: implement 4D move dispatch, or reject unsupported dimensions before the run starts.include/Metropolis.hpp#L293-L303: implement 4D move generation andCalculateA2<4>, or constrain this specialization to 3D.🤖 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/Move_always.hpp` around lines 112 - 120, Prevent exported 4D strategies from silently doing nothing or using 3D logic. In include/Move_always.hpp lines 112-120, add 4D move dispatch or reject unsupported dimensions before execution; in include/Metropolis.hpp lines 293-303, add 4D move generation with CalculateA2<4> or constrain the specialization to 3D. Ensure both affected strategy implementations expose only supported behavior.include/Utilities.hpp (1)
215-296: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove
noexceptfrom the RNG helpers.die_roll,generate_random,make_random_generator,generate_random_int,generate_random_timeslice,generate_random_real, andgenerate_probabilityall seedpcg64fromstd::random_device; if that initialization throws,noexceptturns it intostd::terminateinstead of an exception.🤖 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 215 - 296, Remove the noexcept specifier from the RNG helper functions die_roll, generate_random, make_random_generator, generate_random_int, generate_random_timeslice, generate_random_real, and generate_probability so exceptions from random_device or pcg64 initialization can propagate normally; leave their existing behavior and signatures otherwise unchanged.tests/Ergodic_moves_3_test.cpp (1)
558-563: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKnown-invalid post-flip triangulation is only
WARN-ed here.This scenario builds the
Delaunayfrom raw points without setting vertexinfo()timevalues, then downgrades the validity check toWARNwith a FIXME ("neighbor of c has not c as neighbor"). Note thattests/Bistellar_flip_test.cpp(Lines 119–123) exercises the samebistellar_flipbutREQUIREs success and assertsis_valid()/tds().is_valid()— the difference is that test seeds vertex timevalues viacreate_test_triangulation. So the invariant gap tracked by this FIXME is real and currently uncovered by a hard assertion.Want me to open an issue to track the post-flip neighbor-reciprocity failure so this
WARNcan eventually become aCHECK?🤖 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/Ergodic_moves_3_test.cpp` around lines 558 - 563, Replace the WARN-only validity check in the flipped_triangulation branch with a hard assertion, and ensure the raw-point Delaunay setup initializes vertex info() timevalues consistently with create_test_triangulation before bistellar_flip runs. Preserve the existing post-flip validity and neighbor-reciprocity expectations.Source: Path instructions
.clang-tidy (1)
144-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the clang-tidy option spellings in
.clang-tidy.NamePrefixSuffixSilenceDissimilarityTreshold→NamePrefixSuffixSilenceDissimilarityThreshold,CharTypdefsToIgnore→CharTypedefsToIgnore(both entries), andFowardIt→ForwardItat lines 144, 213, 270, and 347.🤖 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 @.clang-tidy at line 144, Correct the misspelled clang-tidy option keys in .clang-tidy: rename NamePrefixSuffixSilenceDissimilarityTreshold to NamePrefixSuffixSilenceDissimilarityThreshold, CharTypdefsToIgnore to CharTypedefsToIgnore for both entries, and FowardIt to ForwardIt, without changing their values or surrounding configuration.
🤖 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 @.github/workflows/lsan.yml:
- Line 62: Replace the unsupported long sanitizer option with the
CTest-supported short option: update ./initialize --s to ./initialize -s at
.github/workflows/lsan.yml:62-62 and .github/workflows/msan.yml:56-56, and
update ./cdt --s to ./cdt -s at .github/workflows/lsan.yml:70-70 and
.github/workflows/msan.yml:64-64.
In @.github/workflows/valgrind.yml:
- Around line 58-75: Replace the ambiguous `--s` arguments with `-s` in the
Valgrind commands for `initialize` and `cdt`. Leave the existing options and
Valgrind configuration unchanged.
In `@scripts/codecov.sh`:
- Around line 14-18: Add -DCMAKE_BUILD_TYPE=Debug to the cmake configure command
in scripts/codecov.sh, alongside the existing ENABLE_COVERAGE option, so
single-config generators use Debug while preserving the current build and test
commands.
In `@scripts/fast-build.sh`:
- Around line 19-20: After exporting VCPKG_ROOT and running bootstrap-vcpkg.sh
in the fast-build flow, clear the existing out/build/reference cache before
configuration. Mirror the cache-reset behavior used by scripts/build.bat so
stale toolchain and dependency paths cannot be reused.
---
Outside diff comments:
In @.clang-tidy:
- Line 144: Correct the misspelled clang-tidy option keys in .clang-tidy: rename
NamePrefixSuffixSilenceDissimilarityTreshold to
NamePrefixSuffixSilenceDissimilarityThreshold, CharTypdefsToIgnore to
CharTypedefsToIgnore for both entries, and FowardIt to ForwardIt, without
changing their values or surrounding configuration.
In `@include/Metropolis.hpp`:
- Around line 234-239: Update the SIX_TWO branch in the move_tracker action
calculation to reflect removal of four combined (3,1)/(1,3) cells and no (2,2)
cells: decrement the N3_31_13 argument by 4 while leaving N3_22 unchanged, and
preserve the existing N1_TL decrement.
In `@include/Move_always.hpp`:
- Around line 112-120: Prevent exported 4D strategies from silently doing
nothing or using 3D logic. In include/Move_always.hpp lines 112-120, add 4D move
dispatch or reject unsupported dimensions before execution; in
include/Metropolis.hpp lines 293-303, add 4D move generation with CalculateA2<4>
or constrain the specialization to 3D. Ensure both affected strategy
implementations expose only supported behavior.
In `@include/Utilities.hpp`:
- Around line 215-296: Remove the noexcept specifier from the RNG helper
functions die_roll, generate_random, make_random_generator, generate_random_int,
generate_random_timeslice, generate_random_real, and generate_probability so
exceptions from random_device or pcg64 initialization can propagate normally;
leave their existing behavior and signatures otherwise unchanged.
In `@Justfile`:
- Around line 164-180: The _format-check recipe currently compares HEAD with its
worktree, so committed changes are skipped in clean checkouts and CI. Update its
tracked-file validation to compare against the explicit PR/base revision,
ensuring that base revision is available, or otherwise format-check all tracked
C/C++ files directly; preserve the existing untracked-file validation.
In `@tests/Ergodic_moves_3_test.cpp`:
- Around line 558-563: Replace the WARN-only validity check in the
flipped_triangulation branch with a hard assertion, and ensure the raw-point
Delaunay setup initializes vertex info() timevalues consistently with
create_test_triangulation before bistellar_flip runs. Preserve the existing
post-flip validity and neighbor-reciprocity expectations.
🪄 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
Run ID: c2b6a066-85ce-406f-8715-a26fb5b9d420
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.clang-tidy.cmake-format.yaml.coderabbit.yaml.gitattributes.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/actions/setup-just/action.yml.github/workflows/asan.yml.github/workflows/ci.yml.github/workflows/clang-format-check.yml.github/workflows/codecov-upload.yml.github/workflows/codeql.yml.github/workflows/cpp-linter.yml.github/workflows/doxygen.yml.github/workflows/lsan.yml.github/workflows/msan.yml.github/workflows/tsan.yml.github/workflows/valgrind.yml.github/workflows/whitespace.yml.gitignore.python-versionCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/Cache.cmakecmake/Coverage.cmakecmake/Sanitizers.cmakecmake/StandardProjectSettings.cmakecmake/Valgrind.cmakeinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Periodic_3_triangulations.hppinclude/Settings.hppinclude/Torus_d.hppinclude/Utilities.hpppyproject.tomlscripts/asan.shscripts/bootstrap-vcpkg.shscripts/build.batscripts/clang-tidy.shscripts/codecov.shscripts/cppcheck.shscripts/debug.shscripts/fast-build.shscripts/gcov.shscripts/iwyu.shscripts/lsan.shscripts/msan.shscripts/slurm.shscripts/tsan.shsrc/cdt.cppsrc/optimize-initialize.pysrc/test.pytests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/Ergodic_moves_3_test.cpptests/Function_ref_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Settings_test.cpptests/Torus_test.cpptests/Utilities_test.cppty.toml
💤 Files with no reviewable changes (1)
- .cmake-format.yaml
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
- make rejected moves failure-atomic and implement the causal 6→2 retriangulation - correct 6→2 Metropolis action bookkeeping and add cdt --no-output - consolidate LLVM 22 formatting, clang-tidy, sanitizer, and quality workflows - preserve compatible CMake and vcpkg caches across incremental builds - remove cdt-opt, Valgrind integration, and unsupported 4D scaffolding - document the maintenance-mode release workflow BREAKING CHANGE: Remove the cdt-opt executable and unsupported 4D manifold, geometry, move, and cell-type APIs.
- allow clean builds to proceed when no CMake cache exists - reuse pinned vcpkg checkouts containing generated artifacts - install the pinned zizmor version before repository quality checks
Remove the cppcheck workflow, CMake preset, local runner, CodeRabbit configuration, and documentation references.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/Move_command.hpp (2)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
std::cref— same pattern already flagged and fixed elsewhere in this PR.
get_const_results()returnsManifoldType const&, soreturn std::cref(m_manifold);builds an unnecessaryreference_wrapperthat immediately converts back. This is the identical anti-pattern already fixed ininclude/Foliated_triangulation.hpp'sget_delaunay()(per a previous review comment);return m_manifold;is simpler and equivalent here too.🧹 Simplify
auto get_const_results() const -> ManifoldType const& - { return std::cref(m_manifold); } // get_const_results + { return m_manifold; } // get_const_results🤖 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/Move_command.hpp` around lines 74 - 78, Update Move_command.hpp’s get_const_results() to return m_manifold directly instead of wrapping it with std::cref, preserving the declared ManifoldType const& return type.
85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_succeeded()is missing the trailing return type its siblings have, so it returns by value instead of by reference.
get_attempted()andget_failed()both declare-> Counter const&, butget_succeeded()omits the trailing return type. With plainauto(no&), the deduced return type isCounter(a copy of the wholeMoveTracker), notCounter const&. Behaviorally harmless sinceMoveTrackeris cheap to copy, but it's an inconsistent public API surface for three otherwise-parallel accessors.🔧 Align the signature with its siblings
/** * \brief Successful moves by MoveCommand */ - [[nodiscard]] auto get_succeeded() const + [[nodiscard]] auto get_succeeded() const -> Counter const& { return m_succeeded; } // get_succeeded🤖 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/Move_command.hpp` around lines 85 - 101, Update MoveCommand::get_succeeded() to declare the same const-reference return type as get_attempted() and get_failed(), preserving the parallel accessor API and avoiding a returned Counter copy.include/Utilities.hpp (1)
220-231: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
generate_random(anddie_roll) reseed a fresh RNG fromstd::random_deviceon every single call — this is a hot-path perf trap.
generate_random, and by extensiongenerate_random_int,generate_random_real,generate_probability, anddie_roll, each construct a brand-newpcg_extras::seed_seq_from<std::random_device>andpcg64generator per invocation.std::random_devicetypically hits a syscall (/dev/urandom/getrandom()), so this is orders of magnitude slower than drawing from an already-seeded PRNG.make_random_generator()right below already shows the correct pattern (build once, reuse forstd::shuffle), butgenerate_randomdoesn't follow it.This matters because
generate_random_intfeedsmove_tracker::generate_random_move_3(), called once per simplex per pass in the Metropolis/MoveAlways main loops (include/Metropolis.hpp,include/Move_always.hpp), andgenerate_probability()is called once pertry_move(). For large triangulations run over many passes, this reseed-per-draw overhead compounds into a real bottleneck.⚡ Reuse a seeded generator instead of reseeding per call
template <typename NumberType, class Distribution> [[nodiscard]] auto generate_random(NumberType t_min_value, NumberType t_max_value) { - pcg_extras::seed_seq_from<std::random_device> seed_source; - // Make a random number generator - pcg64 generator(seed_source); + thread_local pcg64 generator{pcg_extras::seed_seq_from<std::random_device>{}}; Distribution distribution(t_min_value, t_max_value); return distribution(generator); } // generate_random()The same fix pattern applies to
die_roll().Also applies to: 246-264
🤖 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 220 - 231, Update generate_random and die_roll to reuse a generator that is seeded once rather than constructing pcg_extras::seed_seq_from<std::random_device> and pcg64 on every invocation. Preserve the existing random distributions and return behavior, and ensure generate_random_int, generate_random_real, generate_probability, and die_roll all draw from the reused generator.
🤖 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 @.github/workflows/_sanitizer.yml:
- Around line 45-92: Add a reasonable timeout-minutes setting to the sanitizer
job definition so ASAN, LSAN, MSAN, and TSAN runs are terminated promptly if
they hang. Keep the existing sanitizer steps and configuration unchanged.
In `@include/Ergodic_moves_3.hpp`:
- Around line 577-582: Update the `(6,2)` move-selection logic around
`try_62_move` to iterate through all movable vertex candidates, continuing when
a candidate is rejected instead of returning failure immediately. Apply the
triangulation swap and return `t_manifold` only for the first accepted move;
preserve the existing failure behavior after every candidate has been attempted.
In `@include/Metropolis.hpp`:
- Around line 443-453: Remove the intermediate value copy in the checkpoint
block and pass the result of command.get_results() directly to
utilities::write_file. Keep the existing checkpoint and m_write_files conditions
unchanged, matching the equivalent Move_always.hpp flow while preserving
read-only access.
In `@include/Move_tracker.hpp`:
- Around line 102-111: Add a non-const move_type overload to MoveTracker
alongside the existing const operator[](move_type const) overload, returning a
mutable reference to the corresponding moves entry via as_integer(move).
Preserve the const overload for read-only MoveTracker instances so enum-based
access supports both reads and writes.
In `@scripts/clang-tidy.sh`:
- Around line 11-16: Update the LLVM version lookup in scripts/clang-tidy.sh so
it does not invoke host just before the pkgx fallback in the
CDT_CLANG_TIDY_ACTIVE branch can run. Resolve llvm_version without requiring
installed just, or move/bootstrap the just invocation through pkgx first, while
preserving the existing clang-tidy provisioning behavior.
- Around line 32-34: Update the CMake configure invocation in the clang-tidy
script to pass the cache variable VCPKG_INSTALLED_DIR instead of
_VCPKG_INSTALLED_DIR, using the existing clang_tidy_vcpkg_installed_dir value so
cache refresh logic selects the dedicated installed tree.
In `@scripts/prepare-vcpkg.sh`:
- Around line 7-16: Update the environment-clearing section of prepare-vcpkg.sh
to also unset CC and CXX before the Darwin-specific compiler assignments,
ensuring inherited compiler selectors cannot override the reference toolchain.
In `@src/initialize.cpp`:
- Around line 151-154: Update the invalid_argument message in the dimensions
validation branch of initialize so it accurately describes that unsupported
dimension values other than 3 are rejected, including values below 3, rather
than claiming only values greater than 3 are invalid.
---
Outside diff comments:
In `@include/Move_command.hpp`:
- Around line 74-78: Update Move_command.hpp’s get_const_results() to return
m_manifold directly instead of wrapping it with std::cref, preserving the
declared ManifoldType const& return type.
- Around line 85-101: Update MoveCommand::get_succeeded() to declare the same
const-reference return type as get_attempted() and get_failed(), preserving the
parallel accessor API and avoiding a returned Counter copy.
In `@include/Utilities.hpp`:
- Around line 220-231: Update generate_random and die_roll to reuse a generator
that is seeded once rather than constructing
pcg_extras::seed_seq_from<std::random_device> and pcg64 on every invocation.
Preserve the existing random distributions and return behavior, and ensure
generate_random_int, generate_random_real, generate_probability, and die_roll
all draw from the reused generator.
🪄 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
Run ID: 3ac19b00-ce6b-43e9-8edf-20acb0b2c605
📒 Files selected for processing (51)
.clang-tidy.github/CONTRIBUTING.md.github/workflows/_sanitizer.yml.github/workflows/asan.yml.github/workflows/ci.yml.github/workflows/lsan.yml.github/workflows/msan.yml.github/workflows/tsan.ymlCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/RunCdtNoOutputTest.cmakeinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_tracker.hppinclude/Utilities.hppscripts/build.batscripts/build.shscripts/clang-tidy.shscripts/codecov.shscripts/fast-build.batscripts/fast-build.shscripts/pkgx-build.shscripts/prepare-vcpkg.shscripts/sanitizer.shscripts/slurm.shsrc/CMakeLists.txtsrc/cdt-opt.cppsrc/cdt.cppsrc/initialize.cpptests/Bistellar_flip_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Move_tracker_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Utilities_test.cpptests/Vertex_test.cpp
💤 Files with no reviewable changes (4)
- scripts/slurm.sh
- include/Geometry.hpp
- tests/Move_always_test.cpp
- src/cdt-opt.cpp
|
@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 (1)
scripts/prepare-vcpkg.sh (1)
68-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat missing vcpkg cache entries as stale state
prepare_cmake_cache()only invalidates when a value is present and different, so an incompleteCMakeCache.txtcan keep an outdated vcpkg setup alive. Compare the cache entries unconditionally here, and mirror the same check inscripts/build.batandscripts/fast-build.bat.🤖 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 `@scripts/prepare-vcpkg.sh` around lines 68 - 73, Update the cache validation in prepare_cmake_cache() to invalidate whenever cached_toolchain or cached_installed_dir differs from its expected value, including when either entry is missing. Apply the same unconditional comparisons to the corresponding vcpkg cache checks in scripts/build.bat and scripts/fast-build.bat, preserving the existing stale-cache invalidation behavior.
🤖 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 `@scripts/prepare-vcpkg.sh`:
- Around line 68-73: Update the cache validation in prepare_cmake_cache() to
invalidate whenever cached_toolchain or cached_installed_dir differs from its
expected value, including when either entry is missing. Apply the same
unconditional comparisons to the corresponding vcpkg cache checks in
scripts/build.bat and scripts/fast-build.bat, preserving the existing
stale-cache invalidation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 081c22ba-d458-495d-ba19-2bac9e2a3846
📒 Files selected for processing (8)
.coderabbit.yaml.github/workflows/ci.ymlCMakePresets.jsonJustfileREADME.mdcmake/StaticAnalyzers.cmakescripts/bootstrap-vcpkg.shscripts/prepare-vcpkg.sh
💤 Files with no reviewable changes (3)
- cmake/StaticAnalyzers.cmake
- CMakePresets.json
- .coderabbit.yaml
- Try every movable vertex before rejecting a 6-2 proposal. - Preserve reference semantics for move results and counters. - Bound sanitizer jobs and make clang-tidy provisioning reproducible. - Clarify 3D-only initialization and remove HDF5 from the roadmap.
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 (1)
tests/Move_tracker_test.cpp (1)
87-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the
as_movefallback path.This scenario covers only valid inputs
0–4. Add explicit cases such as-1and5, or define and test rejection behavior, because the current implementation silently maps all other integers tomove_type::FOUR_FOUR. This is based on the suppliedinclude/Move_tracker.hppimplementation. As per path instructions,tests/**/*.cppreviews must ensure comprehensive test coverage and proper doctest practices.🤖 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/Move_tracker_test.cpp` around lines 87 - 103, Extend the “Integer to move type conversion” scenario around as_move to cover out-of-range inputs, including -1 and 5. Assert the current fallback result move_type::FOUR_FOUR for each invalid value, or update the assertions if as_move is changed to reject invalid inputs; retain the existing valid mappings for 0–4.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.
Outside diff comments:
In `@tests/Move_tracker_test.cpp`:
- Around line 87-103: Extend the “Integer to move type conversion” scenario
around as_move to cover out-of-range inputs, including -1 and 5. Assert the
current fallback result move_type::FOUR_FOUR for each invalid value, or update
the assertions if as_move is changed to reject invalid inputs; retain the
existing valid mappings for 0–4.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1beba4ca-dd5b-4408-be92-0e658c7e41aa
📒 Files selected for processing (12)
.github/workflows/_sanitizer.ymlREADME.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_command.hppinclude/Move_tracker.hppscripts/clang-tidy.shscripts/prepare-vcpkg.shsrc/CMakeLists.txtsrc/initialize.cpptests/Move_command_test.cpptests/Move_tracker_test.cpp
💤 Files with no reviewable changes (1)
- README.md
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 (4)
include/Move_command.hpp (2)
171-175: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude successful moves in the post-execution summary.
print_successful()is defined at Lines 217-229, butexecute()prints only attempted and failed counts. Addprint_successful()alongside the other summaries, or remove the helper if successful-count reporting is intentionally handled elsewhere.🤖 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/Move_command.hpp` around lines 171 - 175, Update the post-execution debug summary in execute() to call print_successful() alongside print_attempts() and print_errors(), ensuring successful moves are included in the reported counts.
37-40: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDrain or remove the retry queue.
Both failure paths append to
m_moves_to_retry, butexecute()only drainsm_moves. The private retry queue is never retried, exposed, or cleared; because the same command is reused across passes ininclude/Metropolis.hppLines 393-459, failed entries accumulate indefinitely. This also does not implement the PR objective of retrying movable vertices for 6-2 proposals.Implement a bounded retry policy that consumes and clears the queue, or remove the queue and its retry semantics.
Also applies to: 158-166
🤖 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/Move_command.hpp` around lines 37 - 40, Update Move_command’s retry handling so m_moves_to_retry is either consumed and cleared by execute() under a bounded retry policy, or removed together with all retry semantics. Ensure failed entries do not accumulate across reused command executions and that movable vertices for 6-2 proposals are actually retried.tests/Move_tracker_test.cpp (1)
118-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCheck each counter, not only the aggregate.
Line 119 only proves that the sum is zero; it would still pass if nonzero counters cancelled out. Iterate over
moves_view()and assert every element is zero.Suggested assertion
THEN("Each element is zero-initialized.") - { REQUIRE_EQ(tracked_moves.total(), 0); } + { + REQUIRE_EQ(tracked_moves.total(), 0); + for (auto const value : tracked_moves.moves_view()) + { + CHECK_EQ(value, 0); + } + }As per path instructions,
tests/**/*.cpprequires comprehensive test coverage and proper doctest 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/Move_tracker_test.cpp` around lines 118 - 119, Update the test case “Each element is zero-initialized” to iterate through the collection returned by moves_view() and assert every counter is zero individually, rather than checking only tracked_moves.total(). Preserve the existing doctest structure and initialization coverage.Source: Path instructions
tests/Move_command_test.cpp (1)
47-50: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winVerify the entire manifold is unchanged after a failed move.
The failure branch checks only
result.simplices(). A failed topology operation could leave the same simplex count while changing connectivity or metadata, so this helper would miss a violation of the atomic-discard contract ininclude/Move_command.hppLines 66-68. Compare the full manifold state, or all relevant invariants, withbeforewhen the move fails.🤖 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/Move_command_test.cpp` around lines 47 - 50, Update the failed-move branch in the test helper around result.simplices() to compare the complete manifold state against before, not just the simplex count. Include connectivity, metadata, and all relevant invariants needed to verify the atomic-discard contract while preserving the existing success-branch checks.
🤖 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/Move_command.hpp`:
- Around line 171-175: Update the post-execution debug summary in execute() to
call print_successful() alongside print_attempts() and print_errors(), ensuring
successful moves are included in the reported counts.
- Around line 37-40: Update Move_command’s retry handling so m_moves_to_retry is
either consumed and cleared by execute() under a bounded retry policy, or
removed together with all retry semantics. Ensure failed entries do not
accumulate across reused command executions and that movable vertices for 6-2
proposals are actually retried.
In `@tests/Move_command_test.cpp`:
- Around line 47-50: Update the failed-move branch in the test helper around
result.simplices() to compare the complete manifold state against before, not
just the simplex count. Include connectivity, metadata, and all relevant
invariants needed to verify the atomic-discard contract while preserving the
existing success-branch checks.
In `@tests/Move_tracker_test.cpp`:
- Around line 118-119: Update the test case “Each element is zero-initialized”
to iterate through the collection returned by moves_view() and assert every
counter is zero individually, rather than checking only tracked_moves.total().
Preserve the existing doctest structure and initialization coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8489374c-ce1b-4e93-82b2-49bcd8a14b54
📒 Files selected for processing (12)
.github/workflows/_sanitizer.ymlREADME.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_command.hppinclude/Move_tracker.hppscripts/clang-tidy.shscripts/prepare-vcpkg.shsrc/CMakeLists.txtsrc/initialize.cpptests/Move_command_test.cpptests/Move_tracker_test.cpp
💤 Files with no reviewable changes (1)
- README.md
- invalidate incomplete CMake/vcpkg configuration state on Unix and Windows - report successful queued moves and remove unused retry bookkeeping
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 (2)
tests/Move_command_test.cpp (1)
100-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew state-preservation checks on failure are thorough.
The addition of
get_delaunay()equality,vertex_states,cell_states, andmanifold_countscomparisons in the failure branch provides excellent coverage for the rejection invariant. The new "Rejected moves preserve manifold state" scenario (lines 429-457) complements this well with a minimal manifold that forces rejection.One minor note: the parameter named
moveon line 102 shadowsstd::movedue tousing namespace std;on line 24. Not a bug sincestd::moveisn't used in this function, but renaming tomove_typeort_movewould avoid potential confusion during future edits.♻️ Suggested rename
- void check_single_move_outcome(MoveCommand<Manifold_3> const& command, - Manifold_3 const& before, - move_tracker::move_type const move, - Int_precision const cell_delta) + void check_single_move_outcome(MoveCommand<Manifold_3> const& command, + Manifold_3 const& before, + move_tracker::move_type const t_move, + Int_precision const cell_delta) { - auto const attempted = command.get_attempted()[move]; - auto const succeeded = command.get_succeeded()[move]; - auto const failed = command.get_failed()[move]; + auto const attempted = command.get_attempted()[t_move]; + auto const succeeded = command.get_succeeded()[t_move]; + auto const failed = command.get_failed()[t_move]; CHECK_EQ(attempted, 1); CHECK_EQ(succeeded + failed, 1); auto const& result = command.get_const_results(); CHECK(result.is_valid()); if (succeeded == 1) { CHECK_EQ(result.simplices(), before.simplices() + cell_delta); - CHECK(ergodic_moves::check_move(before, result, move)); + CHECK(ergodic_moves::check_move(before, result, t_move)); }🤖 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/Move_command_test.cpp` around lines 100 - 125, Rename the move parameter in check_single_move_outcome from move to a non-conflicting name such as move_type or t_move, and update its uses when indexing get_attempted(), get_succeeded(), get_failed(), and calling ergodic_moves::check_move.include/Move_command.hpp (1)
195-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate duplication across print methods.
print_attempts,print_successful, andprint_errorsshare near-identical format strings differing only in the adjective (attempted/successful/failed) and the counter source. Extracting a common helper eliminates ~30 lines of duplicated formatting logic and ensures any future format changes apply uniformly.♻️ Proposed refactor
+ /** + * \brief Print move counts with a label + * \param label The adjective describing the move counts + * \param counter The counter to print + */ + void print_counter(std::string_view label, Counter const& counter) const + { + fmt::print( + "There were {} {} (2,3) moves and {} {} (3,2) moves and {} " + "{} (2,6) moves and {} {} (6,2) moves and {} {} (4,4) moves.\n", + counter.two_three_moves(), label, + counter.three_two_moves(), label, + counter.two_six_moves(), label, + counter.six_two_moves(), label, + counter.four_four_moves(), label); + } /** * \brief Print attempted moves */ void print_attempts() const - { - fmt::print( - "There were {} attempted (2,3) moves and {} attempted (3,2) moves " - "and {} " - "attempted (2,6) moves and {} attempted (6,2) moves and {} attempted " - "(4,4) " - "moves.\n", - m_attempted.two_three_moves(), m_attempted.three_two_moves(), - m_attempted.two_six_moves(), m_attempted.six_two_moves(), - m_attempted.four_four_moves()); - } + { print_counter("attempted", m_attempted); } /** * \brief Print successful moves */ void print_successful() const - { - fmt::print( - "There were {} successful (2,3) moves and {} successful (3,2) moves " - "and {} " - "successful (2,6) moves and {} successful (6,2) moves and {} " - "successful " - "(4,4) " - "moves.\n", - m_succeeded.two_three_moves(), m_succeeded.three_two_moves(), - m_succeeded.two_six_moves(), m_succeeded.six_two_moves(), - m_succeeded.four_four_moves()); - } + { print_counter("successful", m_succeeded); } /** * \brief Print move errors */ void print_errors() const { if (std::all_of(m_failed.moves_view().begin(), m_failed.moves_view().end(), [](auto const& value) { return value == 0; })) { fmt::print("There were no failed moves.\n"); } else - { - fmt::print( - "There were {} failed (2,3) moves and {} failed (3,2) moves and {} " - "failed (2,6) moves and {} failed (6,2) moves and {} failed (4,4) " - "moves.\n", - m_failed.two_three_moves(), m_failed.three_two_moves(), - m_failed.two_six_moves(), m_failed.six_two_moves(), - m_failed.four_four_moves()); - } + { print_counter("failed", m_failed); } }🤖 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/Move_command.hpp` around lines 195 - 245, Extract the shared move-count formatting from print_attempts, print_successful, and print_errors into a private helper that accepts the adjective and counter source, then have each print method delegate to it. Preserve print_errors’ special no-failed-moves message while using the helper for nonzero failures, and keep all existing move categories and output wording unchanged.
🤖 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/Move_command.hpp`:
- Around line 195-245: Extract the shared move-count formatting from
print_attempts, print_successful, and print_errors into a private helper that
accepts the adjective and counter source, then have each print method delegate
to it. Preserve print_errors’ special no-failed-moves message while using the
helper for nonzero failures, and keep all existing move categories and output
wording unchanged.
In `@tests/Move_command_test.cpp`:
- Around line 100-125: Rename the move parameter in check_single_move_outcome
from move to a non-conflicting name such as move_type or t_move, and update its
uses when indexing get_attempted(), get_succeeded(), get_failed(), and calling
ergodic_moves::check_move.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a86321f5-3360-40d5-a7e5-ffd39e7630f6
📒 Files selected for processing (6)
include/Move_command.hppscripts/build.batscripts/fast-build.batscripts/prepare-vcpkg.shtests/Move_command_test.cpptests/Move_tracker_test.cpp
Consolidate the current development history into main and make main the sole CI pull-request target.
Summary by CodeRabbit
just-based maintenance workflow (build/check/fix, clang-tidy, sanitizers).initializeandcdtCLIs with improved help/version handling and--no-outputmode.cdt-viewerfor rendering saved triangulations.