-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
Protein_ligand graph support & junction tree feature #164
base: master
Are you sure you want to change the base?
Conversation
… pr/yuanqidu/157
Review & discussion needed :D |
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.
LGTM, but I think there's some refactoring to do to avoid duplication
graphein/molecule/graphs.py
Outdated
@@ -20,6 +20,12 @@ | |||
compute_edges, | |||
import_message, | |||
) | |||
from graphein.utils.junction_tree.jt_utils import ( |
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.
I think we can move these utils to graphein.molecule.utils
graphein/molecule/graphs.py
Outdated
@@ -227,3 +233,37 @@ def construct_graph( | |||
g = annotate_edge_metadata(g, config.edge_metadata_functions) | |||
|
|||
return g | |||
|
|||
def construct_junction_tree( | |||
smiles: Optional[str] = None, |
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.
I think using an RDKit mol as the input makes this function more general (this way we can call it for mols parsed from SDF, PDB, Mol2 etc).
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.
I also don't think the input should be optional
try: | ||
from .visualisation import plot_chord_diagram | ||
except ImportError: | ||
pass |
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 should use the import message util from graphein.utils.utils.import_message
from graphein.utils.config import PartialMatchOperator, PathMatchOperator | ||
|
||
|
||
class DSSPConfig(BaseModel): |
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.
Unneeded, can import
executable: str = "mkdssp" | ||
|
||
|
||
class GetContactsConfig(BaseModel): |
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.
Unneeded, can import
@@ -0,0 +1,979 @@ | |||
"""Functions for plotting protein graphs and meshes.""" |
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 probably avoid duplication
graphein/protein_ligand/graphs.py
Outdated
return atomic_df | ||
|
||
|
||
def deprotonate_structure(df: pd.DataFrame) -> pd.DataFrame: |
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 probably a void a lot of duplication here
@@ -0,0 +1,1832 @@ | |||
# %% |
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 avoid duplication
log = logging.getLogger(__name__) | ||
|
||
|
||
def compute_distmat(coords: np.ndarray) -> np.ndarray: |
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 avoid duplication
graphein/molecule/graphs.py
Outdated
else: | ||
g.add_edge(n1, n2, kind={"junction_tree"}) | ||
|
||
return g |
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.
needs newline
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
for more information, see https://pre-commit.ci
Kudos, SonarCloud Quality Gate passed! |
Reference Issues/PRs
What does this implement/fix? Explain your changes
Issue #162 a preliminary version of implementation, comments not edited, a couple of points need to be discussed including multiple TODOs and the choice of rdkit or pdb dataframe for ligand. Basic structures and features work out fine, but may need more testing.
What testing did you do to verify the changes in this PR?
for protein_ligand graph, tests/protein_ligand/test_graphs, right now only basic features are tested, see protein_ligand/config.py
for junction tree feature, tests/molecule/test_graphs, only basic usage is tested