Skip to content

fix: reject aliased cell-list layouts#274

Open
galjos wants to merge 1 commit into
devfrom
fix/cell-list-alias-guard
Open

fix: reject aliased cell-list layouts#274
galjos wants to merge 1 commit into
devfrom
fix/cell-list-alias-guard

Conversation

@galjos

@galjos galjos commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add a runtime guard that rejects cell-list configurations where periodic neighbor offsets can alias the same cell.
  • Add a regression test for the invalid 2-cell layout.
  • Update the cell-list setup fixture to use a valid box/cutoff/cell-count combination.

Root cause

The half-neighbor cell-list stencil assumes enough cells per dimension to represent each neighbor offset without wrapping onto already represented cells. If nCells < 2 * nNeighbourCells + 1, periodic wrapping can alias offsets and make the neighbor list mathematically invalid.

Validation

  • cmake --build build-stochastic-rescale-ci --target testCelllist testCelllistSetup --parallel 4
  • ctest --test-dir build-stochastic-rescale-ci -R '^(testCelllist|testCelllistSetup)$' --output-on-failure

Reference

@galjos galjos requested a review from 97gamjak June 12, 2026 12:57
@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.13%. Comparing base (1d02915) to head (e4b5c19).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #274   +/-   ##
=======================================
  Coverage   88.12%   88.13%           
=======================================
  Files         283      283           
  Lines       10874    10879    +5     
  Branches     3376     3379    +3     
=======================================
+ Hits         9583     9588    +5     
  Misses       1251     1251           
  Partials       40       40           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pq-perf-bot

pq-perf-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

⚡ Performance (instruction count) — ✅ no regressions

per-benchmark breakdown
benchmark base Ir PR Ir Δ
bondedForces 41.46M 41.46M -0.00%
boxTransforms 10.67M 10.67M +0.00%
constraints 11.38M 11.38M +0.00%
coulombKernel 5.80M 5.80M +0.00%
forceKernel 15.33M 15.33M -0.00%
integrator 32.17M 32.17M +0.00%
kinetics 9.53M 9.53M +0.00%
linearAlgebra 2.08M 2.08M +0.00%
nonCoulombPairs 5.48M 5.48M -0.00%
shiftVector 5.81M 5.81M +0.00%
virial 11.63M 11.63M +0.00%

Deterministic callgrind instruction counts vs the base branch; gated at ±2%. Not wall-clock.

@galjos galjos added the bug Something isn't working label Jun 14, 2026
for (size_t i = 0; i < 3; ++i)
if (_nCells[i] < requiredCells[i])
throw CellListException(
"Number of cells per dimension must be at least "

@ape33 ape33 Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make the error message more expressive:
add the corresponding dimension (i=0 -> x, i=1 -> y, i=2 -> z) which throws the error
and also add the possible solutions to the error in the error message, i.e. decreasing the coulomb radius cutoff and/or increasing the number of cells in the cell list approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants