Dimaio/patching - Githubissues

A checking that adds machinery for residue patching and adds terminal variants. Patch definitions look like the following:

  - name:  CarboxyTerminus
    display_name: cterm
    pattern: '[*][*]C(=O)[{up}]'
    remove_atoms:
    - <{up}>
    - <O1>
    add_atoms:
    - { name: O    ,  atom_type: OOC }
    - { name: OXT  ,  atom_type: OOC }
    modify_atoms: []
    add_connections: []
    add_bonds:
    - [   <C1>,    OXT ]
    - [   <C1>,    O ]
    icoors:
    - { name:     O, source: <O1>, phi:   80.0 deg, theta: 60.0 deg, d: 1.2, parent:  <C1>, grand_parent: <*2>, great_grand_parent: <*1>}
    - { name:   OXT, source: <O1>, phi: -180.0 deg, theta: 60.0 deg, d: 1.2, parent:  <C1>, grand_parent: <*2>, great_grand_parent: O }

Unlike R3, patches are defined based on chemical identity (via smiles string) rather than residue type, and may only reference atoms within the matched pattern. \<XX> is used to refer to an atom in matched string, where XX is the element name and the occurance number of that element (* is any atom).

Additionally this checkin incorporates a few other changes:

remove requirement on sparse (exclusively use scipy.sparse)
few bugfixes for numpy 1.24
supress numba.jit warnings

Two issues that I am not sure how to deal with:

Right now, the code does substructure matching and returns all hits. The problem: if I want a patch to match (say) C-O but only if the O has no protons, there is no way to do so.
The code currently does not handle >1 patch hit per residue (it dies on an assertion failure). The reason: I'm not sure how to deal with things like atom names (if the patch adds an atom named 'X' how do I modify it for each application).

Minor issues (both with patching and chemical.yaml generally):

right now, PDB reader only uses residue matching to determine terminal patches/variants. R3 automatically adds them at N and C term (and optional distance check). Do we want to do this?
res.properties.polymer.mainchain_atoms -> how to deal with this in patches?
connections .. should we do {name} to differentiate from atoms throughout chemical.yaml?
we probably need more expressiveness in patch language:
- change_atom_type (so extra info does not need to get created?)
- applies_to (interacts with properties?)
should tags be unique? e.g., Nterm and NtermPro both tag residues 'XXX:nterm' ... is this proper?
current format doesn't handle >1 bond to same connection ... is that a problem?
finally, patching code is not super efficient (~20ms to patch). Much of the time is spent on graph matching.

Finally, another issue we might we need to tackle:

I'm not sure the "get_default" scheme for the database code is great ... the current setup seems to encourage tmol to know about everything at load time. We should think about this more.

~~Finally, this is currently a WIP ... the machinery works but some of the scorefunctions (fa_dun in particular) do not like patched residues.~~

@aleaverfay

I think with your dunbrack/packing fixes this is ready to go.

Remaining failures (cpu only, some of these tests also have cuda versions failing on same error), grouped together with a few comments for each:

These are related to fingerprinting:

FAILED tmol/tests/pack/rotamer/test_mainchain_fingerprint.py::test_merge_fingerprints - AssertionError: assert '1HA' not in frozendict.frozendict({'N': 0, 'CA': 1, 'C': 2, 'O': 3, 'H': 4, '1HA': 5, '2HA': 6})
FAILED tmol/tests/pack/rotamer/dunbrack/test_dunbrack_chi_sampler.py::test_package_samples_for_output[cpu] - RuntimeError: shape mismatch: value tensor of shape [12] cannot be broadcast to indexing result of shape [11]
FAILED tmol/tests/pack/sim_anneal/test_sim_anneal.py::test_accept_final_smoke[cpu] - AssertionError
- In tmol/tests/pack, I changed some tests to not use terminal variants (letting them pass). Once fixed, it might be good to test with terminal variants? Search for 'fd' in the test code to see what I changed.

This seems like an indexing error? block_coord_offset seems to be the wrong value when I change to the terminal variant:

FAILED tmol/tests/pose/test_pose_stack_kinematics.py::test_get_bonds_for_named_torsions[cpu] - AssertionError:

Going through the tests, I noticed there was no test that compared fa_dun energies to a reference value (as with other score terms). Dunbrack is currently "working", but the scores are incorrect:

old version: scored with arbitrary phi (nterm) / psi (cterm) values (I think the default from params file)
current version: not scored
desired behavior: scored with "neutral" values (something like -100/100 would be my guess as it does not badly clash with any setting of the "other" value)

One final comment: a lot of tests had to be updated because they were checking the size of some data structure that was keyed off the number of atoms in a pose or residue (e.g., assert coords.shape == (2, 1472, 3)). This seems kind of annoying -- if we ever change the # of atoms in a pose for some reason, a bunch of tests will "fail" for no reason. It's probably not worth going back, but it might be useful in the future for such things to compare against (say) the number of atoms in the input pose or restype rather than a hard-coded value?

Codecov Report

Patch coverage: 93.72% and project coverage change: -0.04% :warning:

Comparison is base (98d1046) 95.13% compared to head (759590a) 95.09%. Report is 4 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #261 +/- ## ========================================== - Coverage 95.13% 95.09% -0.04% ========================================== Files 327 330 +3 Lines 20290 20743 +453 ========================================== + Hits 19303 19726 +423 - Misses 987 1017 +30 ``` | Flag | Coverage Δ | | |---|---|---| | _shrug_Testing_CPU | `89.76% <93.72%> (+0.08%)` | :arrow_up: | | _shrug_Testing_CPU_w_o_jit | `91.66% <93.72%> (+0.03%)` | :arrow_up: | | _shrug_Testing_CUDA | `92.33% <93.72%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files Changed](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | Coverage Δ | | |---|---|---| | [tmol/pack/packer\_task.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC9wYWNrL3BhY2tlcl90YXNrLnB5) | `90.00% <ø> (-2.00%)` | :arrow_down: | | [tmol/system/io.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC9zeXN0ZW0vaW8ucHk=) | `88.46% <ø> (ø)` | | | [tmol/tests/pack/rotamer/test\_bounding\_spheres.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9wYWNrL3JvdGFtZXIvdGVzdF9ib3VuZGluZ19zcGhlcmVzLnB5) | `100.00% <ø> (ø)` | | | [tmol/tests/score/bonded\_atom/test\_bonded\_atom.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9zY29yZS9ib25kZWRfYXRvbS90ZXN0X2JvbmRlZF9hdG9tLnB5) | `100.00% <ø> (ø)` | | | [tmol/tests/score/cartbonded/test\_script\_modules.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9zY29yZS9jYXJ0Ym9uZGVkL3Rlc3Rfc2NyaXB0X21vZHVsZXMucHk=) | `100.00% <ø> (ø)` | | | [tmol/tests/score/ljlk/test\_baseline.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9zY29yZS9samxrL3Rlc3RfYmFzZWxpbmUucHk=) | `100.00% <ø> (ø)` | | | [tmol/tests/score/lk\_ball/test\_baseline.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9zY29yZS9sa19iYWxsL3Rlc3RfYmFzZWxpbmUucHk=) | `100.00% <ø> (ø)` | | | [...ol/tests/score/lk\_ball/test\_lk\_ball\_energy\_term.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC90ZXN0cy9zY29yZS9sa19iYWxsL3Rlc3RfbGtfYmFsbF9lbmVyZ3lfdGVybS5weQ==) | `100.00% <ø> (ø)` | | | [tmol/pack/rotamer/mainchain\_fingerprint.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC9wYWNrL3JvdGFtZXIvbWFpbmNoYWluX2ZpbmdlcnByaW50LnB5) | `88.35% <68.42%> (-4.09%)` | :arrow_down: | | [tmol/chemical/restypes.py](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC9jaGVtaWNhbC9yZXN0eXBlcy5weQ==) | `93.19% <68.75%> (-1.86%)` | :arrow_down: | | ... and [35 more](https://app.codecov.io/gh/uw-ipd/tmol/pull/261?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/uw-ipd/tmol/pull/261/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

@aleaverfay I've made all the requested changes. I added an error check on patch charges to fix the order dependency issue.

@aleaverfay Think this is ready for re-review.

uw-ipd / tmol

Dimaio/patching #261

Codecov Report