Fix circular neighbor list copy dependencies and re-enable trimming (issue #4529)

[stanmoore1]

Summary

NeighRequest::cutoff had two possible meanings and no way to tell them apart:

• the maximum cutoff across atom types, where individual type pairs may use a shorter one (what a pair style typically means), or
• a cutoff that applies uniformly to every pair of atom types (what a fix or compute analyzing a fixed range, such as an RDF, typically means).

Because the default neighbor list is built with a cutoff per pair of atom types, guessing wrong either truncates the list for the type pairs with a shorter cutoff, or forces lists to be duplicated where they could be shared. Resolving that ambiguity is what allows the circular copy dependency of issue lammps#4529 to be fixed without reintroducing the truncated analysis lists of issue lammps#4437, and re-enables neighbor list trimming.

This is not a hard pair vs non-pair distinction, nor a perpetual vs occasional one; those are just useful ways to describe the typical case. See Implementation Notes.

The first commit is @jtclemm's, introducing the distinction. The remaining commits complete it:

• Define the missing set_cutoff_min(). fix nonaffine/displacement called it but it was never defined, so EXTRA-FIX did not compile. Its cutoff applies to all types, so it now uses set_cutoff_fixed().
• Replace set_cutoff() and set_cut_fixed() with two single-call setters that each state the intent, set_cutoff_max() and set_cutoff_fixed(), and remove the bare set_cutoff(). All 17 call sites are converted, so leaving the interpretation unstated is a compile error rather than a silently wrong answer. Previously it was an optional second call that was in fact missing at several call sites.
• Clear cut_fixed when morph_unique() drops the cut flag, so a stale interpretation cannot leak into the cutoff comparison that collapses copy chains in morph_copy_trim().
• Restructure the decision in morph_unique() into one explicit predicate. Behavior is unchanged; the deliberate fallthrough that lets a shorter uniform cutoff be trimmed rather than silently widened is now documented instead of implied.
• Order copies by cutoff rather than by the cut flag in morph_copy_trim(), fixing a circular dependency between three or more perpetual lists that share a custom cutoff.

Related Issue(s)

fixes lammps#4529

Also relevant: lammps#4437, whose fix is what this distinction preserves.

Author(s)

Stan Moore (SNL), stanmoore1@gmail.com (corresponding author).

The first commit in this branch is authored by Joel Clemmer (SNL) and is included unchanged.

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Artificial Intelligence (AI) Tools Usage

Portions of this pull request were generated with Claude (Anthropic) under my direction. The cut_fixed distinction in morph_unique() originates from @jtclemm's commit, which is included unchanged and is not AI generated. The AI generated parts are: the set_cutoff_max()/set_cutoff_fixed() API refactor and its call site conversions, the morph_unique() restructuring and flag clearing, the copy cycle fix in morph_copy_trim(), the added unit tests, and the documentation updates. All of it was reviewed by me and verified as described below.

Backward Compatibility

No changes to input scripts or their behavior, other than the bug fixes themselves.

There is an API change for code outside the distribution: NeighRequest::set_cutoff() and NeighRequest::set_cut_fixed() are removed in favor of set_cutoff_max() and set_cutoff_fixed(). This is deliberate, since the removed call had no way to express the interpretation and defaulted to the one that silently truncates. A new section in Developer_updating.rst documents the migration, and the failure mode is a compile error rather than incorrect results.

This is the one point worth an explicit decision before a stable release. A deprecated set_cutoff() alias would preserve source compatibility, at the cost of restoring a default that is silently wrong for uniform-cutoff requests.

Implementation Notes

The two interpretations are the whole point, and the divider is neither pair vs non-pair nor perpetual vs occasional.

set_cutoff_fixed() is used by compute rdf, adf, hbond/local, ave/sphere/atom, composition/atom, efield/wolf/atom, fix rheo/oxidation, fix rheo/thermal, fix hyper/local, fix nonaffine/displacement, fix lambda/la/csp/apip, and the library request path, spanning fixes and computes, perpetual and occasional. set_cutoff_max() is used by pair hybrid, pair hybrid/molecular, pair kim, and pair lj/cut/tip4p/long/gpu.

fix group is the clearest illustration, requesting perpetual or occasional depending on every, from one call site with one interpretation:

if (nevery == 1) req = neighbor->add_request(this, NeighConst::REQ_FULL);
else             req = neighbor->add_request(this, NeighConst::REQ_FULL | NeighConst::REQ_OCCASIONAL);
req->set_cutoff_fixed(cutoff);

The pair vs non-pair framing appears only as the explanatory typical case, in neigh_request.h and Developer_notes.rst.

This also replaces the earlier || irq->occasional test: that clause is gone, and lammps#4437 is handled by the interpretation itself rather than by inferring intent from the occasional flag. A perpetual fix with a uniform cutoff, which the occasional proxy got wrong, is now correct and has a regression test.

Copy cycle fix. morph_copy_trim() orders copies by list index to keep three or more perpetual lists from becoming copies of each other:

if (!irq->occasional && !irq->cut && j > i) continue;

The !irq->cut term stood in for "both lists use the default cutoff", which held while few lists kept a custom cutoff. Once a request keeps its cutoff whenever it differs from cutneighmax, several sub-styles sharing one cutoff all skip the ordering and can form a cycle. With

pair_style hybrid/overlay lj/cut 2.5 lj/cut 1.2 lj/cut 1.2 lj/cut 1.2

initialization failed with "Failed to reorder neighbor lists to satisfy constraints", the very error of lammps#4529, on an input that develop handles. Two equal cutoffs stayed fine, since the existing jrq->copylist == i check catches a 2-cycle, so this only appeared with three or more.

The chain is bounded by cutoff, not by the cut flag: a copy or trim always points at a list whose cutoff is greater or equal, so a strictly longer cutoff makes the cutoff increase along the chain and it can never close. Only equal cutoffs need the index as a tie break:

if (!irq->occasional && (icut == jcut) && j > i) continue;

For lists on the default cutoff icut and jcut are both cutneighmax, so their behavior is unchanged, and trimming from a longer list is still allowed regardless of index.

Verification, on serial, MPI (OpenMPI 4.1.6, up to 8 ranks) and KOKKOS (Serial backend) builds:

• The reproducer in [BUG] Neighbor list logic issue can lead to circular dependencies lammps/lammps#4529 initializes and produces an acyclic set of lists.
• The reproducer in [BUG] Cannot always obtain the correct neighbor list when pair contains different cutoff for different type. lammps/lammps#4437, run unmodified, gives radial distribution functions that reach the requested cutoffs (6.0 and 8.0) instead of being truncated at the 3.6 pair cutoff.
• Trimming works and is exact: the energy of a trimmed hybrid/overlay sub-style matches a standalone pair style at the same cutoff to all printed digits.
• A perpetual fixed cutoff list whose cutoff equals cutneighmax in a system with heterogeneous per-type cutoffs is no longer truncated, and matches develop and an untruncated reference.
• Dynamic load balancing with comm_style tiled and fix balance rcb on 8 ranks, with trimming active, conserves atom count and reproduces the unbalanced energies exactly.
• test_neighbor_class passes on all three builds.

Three tests are added to unittest/cplusplus/test_neighbor_class.cpp, and the two previously disabled one_trim_half_list_{newton,nonewton} tests are re-enabled. The new tests assert through the library interface rather than the printed neighbor list summary, so they do not depend on that formatting. Each was checked to fail without its corresponding fix.

Out of scope: occasional lists influencing perpetual list optimization

Raised in review, and worth recording, but not addressed here because it is pre-existing and independent of the two interpretations above.

cutneighmin folds in the cutoffs of occasional requests (neighbor.cpp, "update cutneighmin based on individual neighbor list requests") and then feeds morph_unique(), morph_copy_trim(), and bin sizing. So an occasional list can change how the perpetual lists are built. With a uniform lj/cut 6.0 and group ... dynamic ... within 6.0 every 1, the perpetual fix GROUP list shares the default binning on its own, but adding an unrelated compute rdf ... cutoff 1.0 gives it cut 6.3 and unique = 1, which skips the bin sharing loop in Neighbor::init() and instantiates a second NBin, binning every atom a second time per reneighboring step.

develop does the same thing, so this branch neither introduces nor fixes it. Fixing it means deriving those decisions from a minimum taken over perpetual requests only, which is a behavior change well beyond this bug fix and belongs in its own pull request.

Post Submission Checklist

• The feature or features in this pull request is complete
• Licensing information is complete
• Corresponding author information is complete
• The source code follows the LAMMPS formatting guidelines
• Suitable new documentation files and/or updates to the existing docs are included
• The added/updated documentation is integrated and tested with the documentation build system
• The feature has been verified to work with the conventional build system
• The feature has been verified to work with the CMake based build system
• Suitable tests have been added to the unittest tree.

Further Information, Files, and Links

Two items are not verified and are left unchecked above: the documentation build was not run, and only the CMake build system was used, not the traditional make build. Both are worth confirming in CI or by a reviewer.

A follow-up branch, neigh_cutoff_min_reuse, builds on this one and adds a third interpretation, set_cutoff_min(), for styles that filter neighbors by distance themselves. It lets such a request reuse the default list when that list already covers the cutoff, and fixes a pre-existing per-type truncation in compute ave/sphere/atom. It is kept separate so this bug fix can be reviewed and backported on its own.