observables: reject nonexistent particle ids (bug-sweep #23) - #5357
Draft
RudolfWeeber wants to merge 1 commit into
Draft
observables: reject nonexistent particle ids (bug-sweep #23)#5357RudolfWeeber wants to merge 1 commit into
RudolfWeeber wants to merge 1 commit into
Conversation
The documented "(existing) particle" precondition for observable ids was enforced by no layer (Python, script interface, or core). Passing a nonexistent id to a get_all_particle_positions-family chain observable (ParticleDistances, BondAngles, BondDihedrals, CosPersistenceAngles, RDF) made fetch_particles silently drop the missing id, so detail::get_argsort produced an out-of-range index that detail::get_all_particle_positions consumed as an out-of-bounds vector read (assert-abort in debug builds, heap garbage or SIGSEGV in release). The Map family (ParticlePositions/ Velocities/Forces) returned silently-wrong values via the same dropped id. Add a collective existence check at the single chokepoint PidObservable::operator(), which runs before fetch_particles() for every particle-based observable. It counts locally-present, non-ghost particles matching the requested ids, all_reduce()s the count across ranks, and if it does not equal ids().size() throws std::runtime_error naming the first missing id. The reductions are collective on every rank, so there is no MPI deadlock, and the exception is surfaced to Python via the script interface's parallel_try_catch wrapper. This mirrors the existing precedent in script_interface/analysis/Analysis.cpp::check_topology. Extend testsuite/python/observable_chain.py with a multi-rank in-process test asserting RuntimeError for a nonexistent id, and a single-rank subprocess crash-safety test that treats an abort/segfault as failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
The documented "(existing) particle" precondition for observable ids was
enforced by no layer (Python, script interface, or core). Passing a
nonexistent id to a get_all_particle_positions-family chain observable
(ParticleDistances, BondAngles, BondDihedrals, CosPersistenceAngles, RDF)
made fetch_particles silently drop the missing id, so detail::get_argsort
produced an out-of-range index that detail::get_all_particle_positions
consumed as an out-of-bounds vector read (assert-abort in debug builds,
heap garbage or SIGSEGV in release). The Map family (ParticlePositions/
Velocities/Forces) returned silently-wrong values via the same dropped id.
Add a collective existence check at the single chokepoint
PidObservable::operator(), which runs before fetch_particles() for every
particle-based observable. It counts locally-present, non-ghost particles
matching the requested ids, all_reduce()s the count across ranks, and if it
does not equal ids().size() throws std::runtime_error naming the first
missing id. The reductions are collective on every rank, so there is no MPI
deadlock, and the exception is surfaced to Python via the script interface's
parallel_try_catch wrapper. This mirrors the existing precedent in
script_interface/analysis/Analysis.cpp::check_topology.
Extend testsuite/python/observable_chain.py with a multi-rank in-process
test asserting RuntimeError for a nonexistent id, and a single-rank
subprocess crash-safety test that treats an abort/segfault as failure.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
🤖 Generated with Claude Code