Skip to content

Enable ligand proton chi sampling by default#396

Merged
kierandidi merged 6 commits into
masterfrom
fix/proton-chi-default
Jul 17, 2026
Merged

Enable ligand proton chi sampling by default#396
kierandidi merged 6 commits into
masterfrom
fix/proton-chi-default

Conversation

@kierandidi

Copy link
Copy Markdown
Collaborator

Summary

  • enable sample_proton_chi=True by default in both Biotite pose-building entry points
  • support the Cartesian product of multiple ligand proton chis and populate their defining atoms
  • order double-digit chi names numerically and add focused OptH/default regression coverage

Known failing ligand and failure

This is intentionally a draft because one end-to-end gate is still failing.

  • Fixture: tmol/tests/data/protein_ligand_test/cif_inputs/ace.ligand.cif
  • Prepared residue: LG1
  • Failing path: default-on pose_stack_from_biotite(..., prepare_ligands=True) after OptHSampler packing
  • Failure: _assert_no_nan_coords() reports NaN coordinates for ligand polar hydrogens HN1, HN2, HN3, HO1, HO2, HO3, and HO4
  • Diagnostic trace: input coordinates and measured input DOFs are finite, while residue-type ideal DOFs are NaN for the traced sampled hydrogens HN1, HN2, HN3, HO2, HO3, and HO4
  • Control: explicit sample_proton_chi=False builds the same ACE/LG1 pose with finite coordinates

No other ligand fixture has been identified as failing yet; ACE/LG1 is the reproduced case currently covered by the end-to-end regression.

Verification

  • pre-commit black and flake8: pass
  • focused default-forwarding, mixed-radix proton-chi, non-finite-DOF, and numeric chi-order tests: pass
  • end-to-end ACE/LG1 default-on pose regression: fails with the NaN atoms listed above

Next step

Repair ligand ideal internal-coordinate construction for sampled polar hydrogens, then require the ACE/LG1 end-to-end regression and CI to pass before marking this PR ready for review.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.80000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.09%. Comparing base (d1be9d6) to head (11d2481).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
tmol/ligand/generated_geometry.py 86.30% 10 Missing ⚠️
tmol/chemical/ideal_coords.py 0.00% 8 Missing ⚠️
tmol/ligand/structure_to_smiles.py 89.83% 6 Missing ⚠️
tmol/io/details/build_missing_leaf_atoms.py 97.93% 2 Missing ⚠️
tmol/ligand/preparation.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
+ Coverage   89.57%   90.09%   +0.51%     
==========================================
  Files         357      358       +1     
  Lines       31088    31373     +285     
==========================================
+ Hits        27847    28265     +418     
+ Misses       3241     3108     -133     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tmol/pack/rotamer/opth_sampler.py Outdated
conf_dofs_kto[dst + 1, :] = orig_dofs_kto[src + 1, :]
copied_dofs = orig_dofs_kto[src + 1, :]
nonfinite = ~torch.isfinite(copied_dofs).all(dim=1)
if torch.any(nonfinite):

@fdimaio fdimaio Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really don't think we want this here

the pose coming into optH should already be complete and well formed.

for proteins, we use 'generate_missing_leaf_atoms' to build missing H subject to ideal geometry, then call this function. ligands should use same logic. my guess is the logic in that function, not here, needs to get fixed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, seems like you changed that by now?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes fixed now

chi_for_rotamers.shape[1],
opth_cache.expanded_samples.shape[1],
)
for chi_col in range(n_chi_cols):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is completely fine and how R3 works. But you might want to talk to @aleaverfay about this, one downside is with large ligands the number of rotamers blows up exponentially. It should be possible to treat each proton chi independently in the packer.

That might be a future effort though, it's fine for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aleaverfay any idea comment on this here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we talked about it at the meeting yesterday, it's a to do but not in this PR

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combinatorics of ligand / sugar hydroxyls was a big motivating factor for talking about blocks rather than residues. "Residue" is a somewhat rigid idea but "block" has no particular connotation to anything biochemical: it's just a unit on which you operate. The plan for hydroxyls is that they will be broken off from the rest of the residues that they come from and treated as independent blocks. Each block will have its own set of rotamers. What we have to create to make that work is:

  1. a way to split a residue into multiple blocks,
  2. a way to update all of the parameters in the scoring layer to reflect this splitting,
  3. a way to convert a PoseStack created with the un-split block types into one with split block types, and
  4. a way to convert a PoseStack back to its un-split block types form to return structures back to the user in a format that they will understand and be willing to work with

It feels like a decent amount of work, but one limiting factor until now has been that we don't even have any non-amino-acid residue types to start working from, so your push on the ligands is a big step forward on getting this implemented

kierandidi and others added 6 commits July 17, 2026 15:37
Wire multi-proton-chi OptH sampling through pose construction and add regression coverage that documents the remaining ligand ideal-DOF failure.
…rate case' corrections in folding near the root. Add a postprocessing step where geometry-based correctings are applied to bond connectivities.
@fdimaio
fdimaio force-pushed the fix/proton-chi-default branch from db6ad1f to 11d2481 Compare July 17, 2026 22:39
@kierandidi
kierandidi marked this pull request as ready for review July 17, 2026 23:21
@kierandidi
kierandidi merged commit 8a891c8 into master Jul 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants