PR #7337 adds a new source/source_cell/module_neighlist implementation and wires it into the LJ solver. This is a useful first serial prototype, but it is not yet a drop-in replacement for the existing source/source_cell/module_neighbor path used by LCAO and related modules. I think we should track the remaining work explicitly before treating module_neighlist as the production neighbor-search backend.
Current scope
- The new object library is built from
bin_manager.cpp and neighbor_search.cpp in source/source_cell/module_neighlist/CMakeLists.txt.
BinManager::build_atom_neighbors performs a 27-bin search around each local atom, so the basic serial cell-list idea is present.
- The only production caller I found is
ESolver_LJ::runner, which constructs NeighborSearch directly and reads neighbor_search.neighbor_list / neighbor_search.all_atoms.
Blocking issues before production use
-
The new path is not integrated with the existing Grid_Driver / atom_arrange::search API.
Most LCAO downstream modules still depend on Grid_Driver::Find_atom, AdjacentAtomInfo, getAdjacentNum(), getAdjacentTau(), and the historical self-last contract. The new NeighborSearch API returns NeighborList indices instead and does not provide AdjacentAtomInfo.
Evidence:
source/source_esolver/esolver_lj.cpp calls NeighborSearch directly.
- Existing LCAO consumers still include
source_cell/module_neighbor/sltk_grid_driver.h and call Grid_Driver::Find_atom.
- The old contract appends the center atom at the end of the returned list in
sltk_grid_driver.cpp.
Suggested acceptance criterion: either add a compatibility adapter behind Grid_Driver / atom_arrange::search, or clearly document module_neighlist as LJ-only until that adapter exists.
-
MPI domain decomposition is not implemented yet.
NeighborSearch::init accepts mpi_rank, but mpi_size is currently hard-coded to 1, and there are no MPI_* calls or halo exchanges in module_neighlist.
Evidence:
NeighborSearch::init: int mpi_size = 1;
rg "MPI_|MPI\\b|MPI_Cart|halo" source/source_cell/module_neighlist finds no real MPI implementation.
Suggested acceptance criterion: keep the serial path clearly separated from any future distributed path, and add tests that fail if mpi_rank != 0 is accidentally treated as real decomposition without matching mpi_size / halo data.
-
Periodic image metadata is not preserved in the new public result.
Existing callers receive neighbor identity as (type, atom index, box/cell image) plus coordinates. NeighborAtom currently stores position, atom type, atom index, and atom id, but not the image offset (cell_x, cell_y, cell_z). This makes it hard to preserve AdjacentAtomInfo::box semantics.
Suggested acceptance criterion: carry image offsets through NeighborAtom and verify that converted AdjacentAtomInfo::box matches the old implementation on PBC edge cases.
-
The implementation still expands all periodic images before binning.
NeighborSearch::setMemberVariables builds all_atoms by looping over all image layers, then init filters into inside_atoms and ghost_atoms. This is an improvement over scanning every image for every center atom, but it does not yet address the memory side of the old implementation.
Suggested acceptance criterion: add a benchmark that reports peak candidate atoms / memory and compare old module_neighbor vs new module_neighlist for increasing cutoffs and atom counts.
-
Tests do not yet prove equivalence with the existing implementation.
Current tests cover small synthetic cases, allocator behavior, and simple binning. I do not see:
- brute-force or old-implementation equivalence tests,
- PBC edge/corner tests with expected image offsets,
- self-last compatibility tests,
- LJ regression comparison against the previous
Grid_Driver path,
- MPI equivalence tests.
Suggested acceptance criterion: add a brute_force_compare test that compares neighbor sets (type, atom index, image offset) for orthogonal and skewed cells, plus an LJ regression showing energy/force/stress are unchanged relative to the old path.
-
The NeighborList page allocator can silently drop large neighbor lists.
PageAllocator::allocate(n) returns nullptr when n > pgsize. BinManager::build_atom_neighbors then sets numneigh[i] = n even if allocation failed; in release builds, the assert(ptr != nullptr) inside the copy loop may be disabled, leaving firstneigh[i] == nullptr with a positive neighbor count.
Suggested acceptance criterion: either size pages to the maximum observed neighbor count before allocation or make oversize allocation a hard error independent of assert.
Proposed follow-up PR sequence
- Add correctness tests and an adapter that converts
NeighborSearch output to AdjacentAtomInfo while preserving the self-last contract.
- Add old-vs-new equivalence tests for LJ and representative PBC cells.
- Add a reproducible benchmark harness for
Grid::init, neighbor construction time, total neighbor count, and peak memory.
- Only after the serial adapter is proven, consider wiring the new backend into
atom_arrange::search as an opt-in path.
- Implement MPI domain decomposition and halo exchange in a separate PR with 1/2/4/8-rank equivalence tests.
Short PR comment version
This looks like a useful serial cell-list prototype, but it should not yet be treated as a replacement for module_neighbor. The current implementation is LJ-only, bypasses the Grid_Driver / AdjacentAtomInfo API used by LCAO modules, hard-codes mpi_size = 1, carries no image-offset metadata in NeighborAtom, and lacks equivalence tests against the old implementation. I suggest tracking a follow-up issue for a compatibility adapter, PBC/image-offset correctness tests, LJ old-vs-new regression tests, and real benchmark data before expanding this beyond the LJ path.
PR #7337 adds a new
source/source_cell/module_neighlistimplementation and wires it into the LJ solver. This is a useful first serial prototype, but it is not yet a drop-in replacement for the existingsource/source_cell/module_neighborpath used by LCAO and related modules. I think we should track the remaining work explicitly before treatingmodule_neighlistas the production neighbor-search backend.Current scope
bin_manager.cppandneighbor_search.cppinsource/source_cell/module_neighlist/CMakeLists.txt.BinManager::build_atom_neighborsperforms a 27-bin search around each local atom, so the basic serial cell-list idea is present.ESolver_LJ::runner, which constructsNeighborSearchdirectly and readsneighbor_search.neighbor_list/neighbor_search.all_atoms.Blocking issues before production use
The new path is not integrated with the existing
Grid_Driver/atom_arrange::searchAPI.Most LCAO downstream modules still depend on
Grid_Driver::Find_atom,AdjacentAtomInfo,getAdjacentNum(),getAdjacentTau(), and the historical self-last contract. The newNeighborSearchAPI returnsNeighborListindices instead and does not provideAdjacentAtomInfo.Evidence:
source/source_esolver/esolver_lj.cppcallsNeighborSearchdirectly.source_cell/module_neighbor/sltk_grid_driver.hand callGrid_Driver::Find_atom.sltk_grid_driver.cpp.Suggested acceptance criterion: either add a compatibility adapter behind
Grid_Driver/atom_arrange::search, or clearly documentmodule_neighlistas LJ-only until that adapter exists.MPI domain decomposition is not implemented yet.
NeighborSearch::initacceptsmpi_rank, butmpi_sizeis currently hard-coded to1, and there are noMPI_*calls or halo exchanges inmodule_neighlist.Evidence:
NeighborSearch::init:int mpi_size = 1;rg "MPI_|MPI\\b|MPI_Cart|halo" source/source_cell/module_neighlistfinds no real MPI implementation.Suggested acceptance criterion: keep the serial path clearly separated from any future distributed path, and add tests that fail if
mpi_rank != 0is accidentally treated as real decomposition without matchingmpi_size/ halo data.Periodic image metadata is not preserved in the new public result.
Existing callers receive neighbor identity as
(type, atom index, box/cell image)plus coordinates.NeighborAtomcurrently stores position, atom type, atom index, and atom id, but not the image offset(cell_x, cell_y, cell_z). This makes it hard to preserveAdjacentAtomInfo::boxsemantics.Suggested acceptance criterion: carry image offsets through
NeighborAtomand verify that convertedAdjacentAtomInfo::boxmatches the old implementation on PBC edge cases.The implementation still expands all periodic images before binning.
NeighborSearch::setMemberVariablesbuildsall_atomsby looping over all image layers, theninitfilters intoinside_atomsandghost_atoms. This is an improvement over scanning every image for every center atom, but it does not yet address the memory side of the old implementation.Suggested acceptance criterion: add a benchmark that reports peak candidate atoms / memory and compare old
module_neighborvs newmodule_neighlistfor increasing cutoffs and atom counts.Tests do not yet prove equivalence with the existing implementation.
Current tests cover small synthetic cases, allocator behavior, and simple binning. I do not see:
Grid_Driverpath,Suggested acceptance criterion: add a
brute_force_comparetest that compares neighbor sets(type, atom index, image offset)for orthogonal and skewed cells, plus an LJ regression showing energy/force/stress are unchanged relative to the old path.The
NeighborListpage allocator can silently drop large neighbor lists.PageAllocator::allocate(n)returnsnullptrwhenn > pgsize.BinManager::build_atom_neighborsthen setsnumneigh[i] = neven if allocation failed; in release builds, theassert(ptr != nullptr)inside the copy loop may be disabled, leavingfirstneigh[i] == nullptrwith a positive neighbor count.Suggested acceptance criterion: either size pages to the maximum observed neighbor count before allocation or make oversize allocation a hard error independent of
assert.Proposed follow-up PR sequence
NeighborSearchoutput toAdjacentAtomInfowhile preserving the self-last contract.Grid::init, neighbor construction time, total neighbor count, and peak memory.atom_arrange::searchas an opt-in path.Short PR comment version
This looks like a useful serial cell-list prototype, but it should not yet be treated as a replacement for
module_neighbor. The current implementation is LJ-only, bypasses theGrid_Driver/AdjacentAtomInfoAPI used by LCAO modules, hard-codesmpi_size = 1, carries no image-offset metadata inNeighborAtom, and lacks equivalence tests against the old implementation. I suggest tracking a follow-up issue for a compatibility adapter, PBC/image-offset correctness tests, LJ old-vs-new regression tests, and real benchmark data before expanding this beyond the LJ path.