uw-ipd / tmol

TMol
Apache License 2.0
30 stars 3 forks source link

Fix bug in unresolved atom id resolution #267

Closed aleaverfay closed 1 year ago

aleaverfay commented 1 year ago

The unresolved-atom-id resolution code should also handle cases when UAIDs specify the "this is not a real UAID" condition. Previously, it just accessed out of bounds memory locations. Oops.

I have now added unit tests for UAID resolution as well. This test hits the case where the old resolve_atom_from_uaid function would have read out of bounds (though, that would not necessarily have triggered a failure).

jflat06 commented 1 year ago

Fix looks good!

For the unit test, I can't help but feel that this is treating a symptom rather than the cause.

In my mind, this bug made it in because the new code being added (omega) was not being tested on variety of inputs.

Could we perhaps enhance the 'test_whole_pose_scoring_module_single' (or _10, or add a new '_diverse') functions to test on a variety of structures rather than just UBQ1? For example, even just adding a disulfide structure would expose issues with variable numbers of connections.

This would increase the burden of defining baselines, but it seems worth it to me. I guess we could have a "score_smoke" test that doesn't compare the baselines and is just there to catch proper errors, at least.

aleaverfay commented 1 year ago

More testing would be a good idea in any case. In this instance, though, the error is because there are times when we ask one piece of code to evaluate a term like rama on the n-term and the "phi" for the n-terminal residue type is undefined -- then, when we go to resolve the atoms that define phi for this residue, the uaid for the first phi atom is (-1, -1, -1) and instead of exiting gracefully, the resolve_atom_from_uaid function reads the -1 position from one of the tensors.

(Certain terms should handle this "not a valid uaid" case, e.g. Dunbrack. Those terms will need to intercept the -1 returned by resolve_atom_from_uaid and reinterpret it as specifying the neutral phi.)

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 94.96% and project coverage change: -0.01% :warning:

Comparison is base (3b15d57) 95.19% compared to head (dc83d7e) 95.18%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #267 +/- ## ========================================== - Coverage 95.19% 95.18% -0.01% ========================================== Files 331 336 +5 Lines 20508 21102 +594 ========================================== + Hits 19522 20086 +564 - Misses 986 1016 +30 ``` | Flag | Coverage Δ | | |---|---|---| | _shrug_Testing_CPU | `89.94% <94.96%> (+0.14%)` | :arrow_up: | | _shrug_Testing_CPU_w_o_jit | `91.81% <94.96%> (+0.09%)` | :arrow_up: | | _shrug_Testing_CUDA | `92.46% <94.96%> (+0.06%)` | :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/267?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/267?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/267?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/267?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/267?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/267?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/267?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/267?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/267?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/267?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/267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd#diff-dG1vbC9jaGVtaWNhbC9yZXN0eXBlcy5weQ==) | `93.22% <68.75%> (-1.85%)` | :arrow_down: | | ... and [37 more](https://app.codecov.io/gh/uw-ipd/tmol/pull/267?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/267/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.