uw-ipd / tmol

TMol
Apache License 2.0
30 stars 3 forks source link

Remove old scoring code #282

Closed fdimaio closed 5 months ago

fdimaio commented 10 months ago

Some issues that have come up so far (will be updated as progress continues):

With the latter two it is not clear if these tests are redundant or not. We can look at code coverage to see.

I will next focus on adding a kinematic optimization module

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.14%. Comparing base (ace29a8) to head (3dae8c3). Report is 11 commits behind head on master.

:exclamation: Current head 3dae8c3 differs from pull request most recent head dbdf026. Consider uploading reports for the commit dbdf026 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #282 +/- ## ========================================== - Coverage 94.93% 93.14% -1.80% ========================================== Files 374 297 -77 Lines 23883 19071 -4812 ========================================== - Hits 22673 17763 -4910 - Misses 1210 1308 +98 ``` | [Flag](https://app.codecov.io/gh/uw-ipd/tmol/pull/282/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | Coverage Δ | | |---|---|---| | [_shrug_Testing_CPU](https://app.codecov.io/gh/uw-ipd/tmol/pull/282/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | `87.96% <100.00%> (-2.07%)` | :arrow_down: | | [_shrug_Testing_CPU_debug_w_o_jit](https://app.codecov.io/gh/uw-ipd/tmol/pull/282/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | `90.37% <100.00%> (?)` | | | [_shrug_Testing_CPU_w_o_jit](https://app.codecov.io/gh/uw-ipd/tmol/pull/282/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | `?` | | | [_shrug_Testing_CUDA](https://app.codecov.io/gh/uw-ipd/tmol/pull/282/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uw-ipd) | `90.71% <100.00%> (-1.67%)` | :arrow_down: | 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.

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

jflat06 commented 10 months ago

You should be good to remove the old cartbonded database code:

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/scoring/cartbonded.py#L8-L68

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/scoring/__init__.py#L4

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/scoring/__init__.py#L19

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/scoring/__init__.py#L34-L36

jflat06 commented 10 months ago

The DunbrackParamResolver:

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/score/dunbrack/params.py#L143

Is used by the new dunbrack code, but only a fairly small section of it.

You can see the scope of its usage by my new code here:

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/score/dunbrack/dunbrack_energy_term.py#L46-L51

It isn't immediately clear to me how to extricate the dead code from the ParamResolver, Since the new code calls from_database, which does a bunch of preprocessing of the database, it isn't as simple as removing unused functions. But I'm pretty sure most of this code is unnecessary now.

jflat06 commented 10 months ago

Can we remove all of omega, now that it included in a sub-term of another term? Or is that in another PR? Maybe you have already done this, and I'm reading the diff wrong.

jflat06 commented 10 months ago

This file

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/default/scoring/omega.yaml

and:

https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/scoring/__init__.py#L47

Looks like they can be removed, since you added new versions for the BBdependent version.

jflat06 commented 10 months ago

Can remove: https://github.com/uw-ipd/tmol/blob/0a705c3fa797b0d3ff1258539a3034e8691cff3a/tmol/database/default/scoring/cartbonded.old.yaml along with the other code from cartbonded above.

jflat06 commented 10 months ago

No code removed from tmol/score/hbond/params.py - other terms seemed to either cut this entirely or partially.

fdimaio commented 10 months ago

Thanks! Will make these updates.

fdimaio commented 10 months ago

@jflat06 OK this should be ready for review

aleaverfay commented 10 months ago

This seems fine. It makes me think we ought to come up with some PoseStack-based initialization of the KinForest before we remove the PackedSystem code because we don't want to leave it untested or it will rot; so perhaps getting the PoseStack-based KinForest construction code is a higher priority than I first thought.

jflat06 commented 10 months ago

I'll give my 'approval' on the parts of the code I am familiar with, but there is lots left that I don't feel confident in approving.

I don't know if we want to handle the trimming down of the DunbrackParamResolver, but it looks like that's still untouched.