Skip to content
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

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

yuanqidu
Copy link
Contributor

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

@yuanqidu
Copy link
Contributor Author

Review & discussion needed :D

Copy link
Owner

@a-r-j a-r-j left a 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

@@ -20,6 +20,12 @@
compute_edges,
import_message,
)
from graphein.utils.junction_tree.jt_utils import (
Copy link
Owner

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

@@ -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,
Copy link
Owner

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).

Copy link
Owner

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
Copy link
Owner

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):
Copy link
Owner

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):
Copy link
Owner

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."""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably avoid duplication

return atomic_df


def deprotonate_structure(df: pd.DataFrame) -> pd.DataFrame:
Copy link
Owner

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 @@
# %%
Copy link
Owner

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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can avoid duplication

else:
g.add_edge(n1, n2, kind={"junction_tree"})

return g
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs newline

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants