Repository navigation
Conversation
_overlayPoints lines a residue template up with the atoms already present so that missing atoms can be copied from it. It uses the Kabsch algorithm but left out the step that rules out reflections. When the atoms being lined up are nearly flat, for example just N, CA, C and O, the best fit is often a reflection, and the added atoms then make an L amino acid into its D mirror image. If the fit would be a reflection, flip the sign of the column of u that goes with the smallest singular value. This gives the best fit that is a true rotation. Fits that were already rotations are unchanged. Part of openmm#352, which proposed this fix. Co-authored-by: Sahil Athalye <89952559+Sahil-Athalye@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The OXT and CB tests both loaded 1BHL, deleted atoms through Modeller, and had PDBFixer add them back. Move that into _load1BHL and _deleteAndRebuild, and the OXT lookup into _oxtPositions, so each test only states which atoms it removes and what it checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With one point, _overlayPoints returned the centre of points1 as translate2 and minus points2 as center1, the reverse of what its docstring and the general case return. Callers got the right answer only because the rotation is the identity, so the two translations add up the same either way. Swap them into the documented order. Also correct the documented parameter types: callers pass lists of Vec3 in nanometers, not arrays of Quantity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When N, CA, and C are present, leave O and OXT out of the template fit so their psi-dependent positions cannot tilt a missing side chain to the wrong side. Keep other existing atoms in the fit and preserve the fallback for incomplete backbones. Add regression cases for SER57, VAL72, and THR112 in 1BHL, which rebuild with reversed handedness on the reflection fix alone.
Author
|
This PR is a work in progress – I'll hold off until a decision on #355 has been made. |
Member
|
I'm testing this with the validation script provided in #352, but it isn't passing. Even with these two changes, it still reports residues ending up with the wrong chirality. Do you get the same result? |
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.
WIP.
Generated by AI
Depends on #355; please merge that PR first. Fixes #356.
When
addMissingAtoms()rebuilds a side chain, fitting the residue template to the backbone O can tilt the template enough to put CB on the wrong side of the N–CA–C plane. This still happens with the proper-rotation correction in #355 because the template and residue can have different ψ angles.Exclude O and OXT from
_addAtomsToTopology's fit when N, CA, and C are all present. Any remaining side-chain atoms still contribute to the fit. When the backbone is incomplete, keep using all available matching atoms.Add regression cases that remove and rebuild the side chains of SER57, VAL72, and THR112 in the existing 1BHL fixture. All three return with reversed handedness on #355 alone and retain their original handedness with this fix. The existing handedness helper now accepts a residue selector so these tests and the alanine test use the same calculation.
Stack and review
This branch is one commit above #355. Review only this layer.
The PR targets
masterbecause the parent branch exists only in the contributor's fork and cannot serve as an upstream base branch. Until #355 merges, the main PR diff also includes that dependency. This PR is a draft pending #355; after it merges, the branch can be rebased ontomasterto leave only this change.Validation
python -m pytest -q pdbfixer— 29 passed (Python 3.12.11, OpenMM 8.6.1, NumPy 2.5.4, macOS arm64).This addresses template alignment. The separate minimization problem that can flip or flatten residues remains tracked in #352.