-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Torsion Distribution plotting/analysis #20
base: main
Are you sure you want to change the base?
Conversation
Hello @RiesBen! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
=======================================
Coverage ? 47.01%
=======================================
Files ? 8
Lines ? 368
Branches ? 0
=======================================
Hits ? 173
Misses ? 195
Partials ? 0 ☔ View full report in Codecov by Sentry. |
List[Chem.Bond] | ||
List of rdkit.Chem.Bond that were found in a molecule taking symmetry into account. | ||
""" | ||
RotatableBondSmarts = Chem.MolFromSmarts("[!$(*#*)&!D1]-&!@[!$(*#*)&!D1]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from the definition in Lipinski?
List of atomic ids that specify a torsion around the bond of interest. | ||
""" | ||
bond_atoms = [bond.GetBeginAtom(), bond.GetEndAtom()] | ||
additional_atom1 = list(filter(lambda x: x.GetIdx() != bond_atoms[1].GetIdx(), bond_atoms[0].GetNeighbors()))[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to ignore hydrogen torsions right? Does slicing the zeroth element here potentially miss a heavy atom torsion?
@@ -0,0 +1,333 @@ | |||
# This file is thanks Christian W. Feldman |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put plotting into openfe main repo please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could actually be a dependency, it's vendored code:
https://github.com/c-feldmann/lassohighlight/tree/master
|
||
def calculate_dihedrals(mol, torsion_ids_list)->np.array: | ||
dihedrals = [] | ||
for c in mol.GetConformers(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work for our trajectories right?
Original Task description #11 : Type |
Those plots look really nice 🤩 |
@RiesBen Remove the hydrogens. |
@jameseastwood, @hannahbaumann and @IAlibay we should reassign this issue. The implementation should be stable, it is mainly about adressing the comments and merging it. |
This PR contains the Torsion Distribution plotting/analysis for for openFE trajectories.
Features:
Todo:
Example plots: