Repository navigation
Preserve atoms in fragmented GFN-FF Hessians - #1432
Merged
thfroitzheim merged 3 commits intoOct 5, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes GFN-FF fragmented Hessian partitioning to be translation-invariant and to avoid dropping atoms/modes when grid-based merging produces oversized, disconnected blocks (as in issue #1286). It refines grid bound computation and adds validation/fallback behavior, plus a unit test covering the regression scenario.
Changes:
- Compute grid bounds only from active small fragments to remove coordinate-origin dependence.
- Prevent merged Hessian blocks from exceeding the fragment size limit and skip already-merged fragments during grouping.
- Validate that every atom is assigned exactly once; if not, disable fragmentation and fall back to full diagonalization.
- Add a unit test to verify translation invariance and ensure splitting a crowded grid cell doesn’t lose atoms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/gfnff/frag_hess.f90 |
Makes fragmentation translation-invariant, caps merging to maxmagnat, skips merged fragments, and adds partition validity checks with fallback. |
test/unit/test_gfnff.f90 |
Adds a unit test for translation-invariant fragmentation and for preserving atoms when splitting crowded grid cells. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
TyBalduf
force-pushed
the
fix/issue-1286-fragmented-hessian
branch
from
August 10, 2026 17:22
917dd55 to
5913dfd
Compare
Fragment grid bounds included unused zero-initialized center entries, making the partition depend on the coordinate origin. For translated clusters this could place many disconnected molecules in one oversized block; the connectivity splitter then retained only its two endpoint components, leaving most atoms without Hessian modes. Compute bounds from active small fragments, cap merged blocks at the fragment size limit, and skip consumed group leaders. Validate that every atom occurs exactly once and fall back to full diagonalization for an invalid partition. Add regression coverage for translation invariance and crowded grid cells. Fixes grimme-lab#1286 Signed-off-by: Ty Balduf <ty.balduf@schrodinger.com>
The fragmenter populated rmaxab only for atom pairs within the same Hessian block before copying the entire array. Uninitialized cross-block entries can be signaling NaNs in debug builds and trigger an invalid floating-point exception. Initialize those entries to a large distance, which also represents their intended disconnected graph state. Signed-off-by: Ty Balduf <ty.balduf@schrodinger.com>
Signed-off-by: Ty Balduf <ty.balduf@schrodinger.com>
TyBalduf
force-pushed
the
fix/issue-1286-fragmented-hessian
branch
from
August 10, 2026 17:26
5913dfd to
35d612b
Compare
thfroitzheim
approved these changes
Oct 5, 2026
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
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.
Fixes #1286
Fragment grid bounds included unused zero-initialized center entries, making the partition depend on the coordinate origin. For translated clusters (like in #1286 where many of the waters were displace 300 Angstroms away from the origin) this could place many disconnected molecules in one oversized block; the splitter for overly large groups assumes they are fully connected, which leads to it only retain the two endpoint components (disconnected waters), leaving the atoms from all the other waters without Hessian modes.
This change computes bounds based on active small fragments, caps merged blocks at the fragment size limit, and ensures already group fragments are skipped. We now check that every atom belongs to exactly one fragment and fall back to full diagonalization if that check fails.