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

Graph plots #197

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

Graph plots #197

wants to merge 29 commits into from

Conversation

kamurani
Copy link
Contributor

@kamurani kamurani commented Jul 22, 2022

Reference Issues/PRs

couldn't find an issue that references this

What does this implement/fix? Explain your changes

add support for colouring / sizing asteroid plot nodes by different features. Extra functions can be added for more features if it is known how to reference the node data e.g. g.nodes(data=True)[k]['rsa']

change colourscheme to viridis (can revert this if it's unwanted)

What testing did you do to verify the changes in this PR?

Plotted example graphs.

Pull Request Checklist

  • Added a note about the modification or contribution to the ./CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./graphein/tests/* directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under ./notebooks/ (if applicable)
  • Ran python -m py.test tests/ and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., python -m py.test tests/protein/test_graphs.py)
  • Checked for style issues by running black . and isort .

@kamurani
Copy link
Contributor Author

kamurani commented Jul 22, 2022

Testing asteroid plot, scaling by RSA and colour by hydrophobicity

fig1 = asteroid_plot(
    g=g,
    node_id=node,
    colour_nodes_by="hydrophobicity",
    size_nodes_by='rsa',
    node_size_multiplier=80
)

image

@a-r-j a-r-j marked this pull request as ready for review July 22, 2022 09:10
@kamurani
Copy link
Contributor Author

@a-r-j any changes you want me to make?

@a-r-j
Copy link
Owner

a-r-j commented Jul 26, 2022

@a-r-j any changes you want me to make?

Hey, sorry - busy period. A few minor comments but otherwise LGTM. Thanks for the contributions!

graphein/protein/visualisation.py Outdated Show resolved Hide resolved
return hmap[res]

node_colours = []
for n in subgraph.nodes():
Copy link
Owner

Choose a reason for hiding this comment

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

I believe you can replace this nested loop with:

for n, d in subgraph.nodes(data=True)
...

graphein/protein/visualisation.py Outdated Show resolved Hide resolved
graphein/protein/features/nodes/amino_acid.py Outdated Show resolved Hide resolved
graphein/protein/features/nodes/amino_acid.py Outdated Show resolved Hide resolved
graphein/protein/visualisation.py Outdated Show resolved Hide resolved
graphein/protein/features/nodes/amino_acid.py Outdated Show resolved Hide resolved
TODO: Functino that gets ``min`` and ``max`` values in a graph for a given feature so that we can scale / offset to > 0
TODO: should feature `distance` actually contain the site itself i.e. in the string?
"""
def _node_feature_func(
Copy link
Owner

Choose a reason for hiding this comment

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

This is really elegant work - thanks a lot!

Having taken a closer look at the code I wonder if we can be even smarter here. Given the rest of Graphein is quite functional in style, we could perhaps refactor out the sizing functions into atomic functions & then allow users to pass in either a function of their own or a string which would dispatch one the lambdas/more complicated functions you've implemented here. I think this provides the most flexibility whilst supporting all the great features you've added here. What do you think?

@sonarcloud
Copy link

sonarcloud bot commented Aug 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
10.5% 10.5% Duplication

@a-r-j
Copy link
Owner

a-r-j commented Oct 23, 2022

.Hey @kamurani any chance you can approve this PR so we can get this merged in? Thanks!!

@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
5.8% 5.8% Duplication

@a-r-j a-r-j mentioned this pull request Jan 25, 2023
@sonarcloud
Copy link

sonarcloud bot commented Feb 16, 2023

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 7 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

Copy link

sonarcloud bot commented Mar 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link

sonarcloud bot commented Oct 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

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