fix dihedral_backbone() returning garbage angles across chain breaks - #933
Conversation
dihedral_backbone() and nucleotide_dihedral_backbone() built their phi/psi/omega (and alpha/beta/gamma/delta/epsilon/zeta) coordinate arrays purely from positional adjacency between residues, with no check that consecutive residues were actually bonded. When two residues were adjacent in the AtomArray but not chemically connected (missing loop, concatenated chains), the dihedrals spanning that gap came out as finite garbage instead of NaN. Add a bond-distance check (C-N for the peptide backbone, O3'-P for the phosphate backbone) using the same 1.2-1.8 A tolerance already used by check_backbone_continuity()/filter_linear_bond_continuity(), and NaN out the angles that span a junction where the distance falls outside that range. Applied to both dihedral_backbone() (fixes biotite-dev#744) and the identical defect in nucleotide_dihedral_backbone(), which was not filed but has the same root cause. Fixes biotite-dev#744.
padix-key
left a comment
padix-key
left a comment
There was a problem hiding this comment.
Hi, sorry for the delayed response, I was busy the last few days.
This looks quite good, I would only clean up the tests a bit.
|
|
||
|
|
||
| @pytest.mark.parametrize("multi_model", [False, True]) | ||
| def test_dihedral_backbone_chain_break(multi_model): |
There was a problem hiding this comment.
The setup of both test function is basically the same, so could you maybe combine them into one function parametrized on ["dihedral_backbone", "nucleotide_dihedral_backbone"]?
| assert np.all(np.isfinite(phi[..., 1])) | ||
| assert np.all(np.isfinite(psi[..., 0])) | ||
| assert np.all(np.isfinite(omega[..., 0])) | ||
| assert np.all(np.isfinite(psi[..., 2])) | ||
| assert np.all(np.isfinite(omega[..., 2])) | ||
| assert np.all(np.isfinite(phi[..., 3])) |
There was a problem hiding this comment.
The order is a bit odd here 😉. I'd say we either sort by angle name or by index.
The two chain break tests differed only in the input structure and the names of the returned angles, so parametrize a single test on dihedral_backbone/nucleotide_dihedral_backbone instead. Look the returned angles up by name so the assertions can be grouped per angle in ascending residue index order.
|
both done. it's one test now, parametrized on the two functions, with the pdb id and the angle names picked per case. the returned angles go into a dict keyed by name, so the asserts group per angle in ascending index order instead of hopping around. full test_geometry.py green locally, 31 passed. |
Merging this PR will improve performance by 11.59%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | benchmark_match_kmer_selection[KmerTable-None] |
279.3 µs | 243.9 µs | +14.51% |
| ⚡ | benchmark_match[KmerTable-None] |
328.2 µs | 295 µs | +11.25% |
| ⚡ | benchmark_match_kmer_selection[BucketKmerTable(10000)-11*11*1*1***111] |
353.2 µs | 320 µs | +10.38% |
| ⚡ | benchmark_match_kmer_selection[BucketKmerTable(10000)-None] |
353.7 µs | 320.7 µs | +10.28% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing steps-re:fix/dihedral-backbone-chain-breaks (74200bb) with main (d9e7f49)
Footnotes
-
14 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
The CodSpeed report here looks like runner noise rather than a real regression. The three flagged benchmarks are all The same pattern shows up elsewhere. #935 reports the same For what it's worth, the added check is one Happy to acknowledge it on CodSpeed instead if you'd rather clear the check that way. |
Indeed, specifically these tests have a noisy run time, where I did not find the cause yet. I acknowledged this regression. |
|
|
b28c764 to
74200bb
Compare
|
Good catch. The remaining pyright failures were six Root cause is the same phantom branch we already annotate on the return statements: I narrowed the indexed results with Verified locally with the pinned |
2 participants
fixes #744
dihedral_backbone()builds its phi/psi/omega coordinate arrays purely by positional adjacency between residues in the AtomArray. it never checks whether two consecutive residues are actually bonded. so if a structure has a missing loop, or you concatenate two unrelated fragments into one AtomArray, the dihedrals spanning that gap come out as finite garbage instead of NaN, exactly like padix-key called out in the issue thread.went with the approach discussed in the thread: check the C-N distance between consecutive residues directly on the coordinates
dihedral_backbone()already extracted, rather than callingcheck_backbone_continuity()separately (which would mean re-filtering the whole array again). used the same 1.2-1.8 A tolerance thatcheck_backbone_continuity()/filter_linear_bond_continuity()already use elsewhere in the codebase, so the threshold is consistent across the library. where the C(i)-N(i+1) distance falls outside that range,psi[i],omega[i], andphi[i+1]get set to NaN since those are the three angles that straddle the junction between residue i and i+1.while testing this i found
nucleotide_dihedral_backbone()has the exact same bug for the O3'-P bond between residues, so I fixed that too with the same approach (epsilon[i],zeta[i],alpha[i+1]get NaN'd on a broken O3'(i)-P(i+1) junction). that one isn't filed anywhere, just the same root cause turning up twice.verification:
test_dihedral_backbone_chain_breakandtest_nucleotide_dihedral_backbone_chain_breaktotests/structure/test_geometry.py. both build a synthetic structure from two real fragments of 1l2y / 4p5j, renumber the second fragment's res_id to be perfectly continuous with the first (so the test can't accidentally pass just because of a res_id jump), then translate it 1000 A away so the two fragments are not bonded. confirmed both tests fail on current main (angles come back as finite numbers) and pass with this fix.tests/structure/suite (4176 passed, 13 skipped, no failures) to check for regressions, plus the existing multi-model, MDTraj-consistency, and barnaba-consistency dihedral tests specifically.disclosure: this fix (code + tests) was put together with AI assistance (Claude), then reviewed and verified by me before opening the PR.