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

Improve plotting.py #64

Merged
merged 18 commits into from
Sep 12, 2022
Merged

Conversation

zhang-ivy
Copy link
Contributor

@zhang-ivy zhang-ivy commented Sep 1, 2022

Description

This PR improves plotting functions in plotting.py. It does not alter any functions in plotlying.py

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Fix formatting of x and y axis labels
  • Add argument (dpi) for controlling the resolution of the plot when saving
  • Allow each edge to be labeled using the names of the nodes involved in the edge. This enhancement was only introduced for DDG plots. Handles both small molecules and protein mutations. Does not work for plotly-generated plots.
  • Use the adjustText package to adjust data labels so that they are not overlapping
  • Make the minimum and maximum axes of the plot larger

Questions

  • Should we merge this into the main repo or should I just merge it in my fork since some of the changes may be specific to my needs I'm working on?

Status

  • Ready to go

@pep8speaks
Copy link

pep8speaks commented Sep 1, 2022

Hello @zhang-ivy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-09-12 20:05:59 UTC

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Sep 1, 2022

@mikemhenry @ijpulidos Can you review this and help me determine whether this a PR worth merging into the main repo or if I should just merge it into my own fork? Some of the changes I made may be specific to my own needs, though I did try to make the changes as general as possible. Specifically, the following change may be useful for others, but I implemented them only for the case that I'm interested in: plotting DDGs (and not DGs) using plotting.py (and not plotly.py):

  • Allow each edge to be labeled using the names of the nodes involved in the edge. This enhancement was only introduced for DDG plots. Handles both small molecules and protein mutations. Does not work for plotly-generated plots.

Additionally, I didn't want the data labels to overlap, so I use the adjustText package to overcome that problem. I'm not sure if that package is something that we want to include as a dependency though.

cinnabar/plotting.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #64 (00d176a) into main (76130d1) will decrease coverage by 2.51%.
The diff coverage is 0.00%.

❗ Current head 00d176a differs from pull request most recent head 31b89be. Consider uploading reports for the commit 31b89be to get more accurate results

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Sep 6, 2022

@mikemhenry : What do you think about including adjustText as a dependency -- are you ok with it? Merging this PR would require it.

@mikemhenry
Copy link
Collaborator

@zhang-ivy thank you for pointing that out! I'm fine with it, given that
https://github.com/conda-forge/adjusttext-feedstock/blob/main/recipe/meta.yaml
it doesn't pull in anything extra and is on conda-forge! (I typed this out so that later I remember why we decided to do this)

@mikemhenry
Copy link
Collaborator

TODO

  • add adjustText to conda env yaml

cinnabar/plotting.py Outdated Show resolved Hide resolved
@zhang-ivy
Copy link
Contributor Author

@mikemhenry : I think I'm done making changes to this! Could you add adjustText to the conda env yaml?

@mikemhenry
Copy link
Collaborator

@zhang-ivy yes! I can take care of this later today

@mikemhenry mikemhenry enabled auto-merge (squash) September 12, 2022 20:07
@mikemhenry mikemhenry self-requested a review September 12, 2022 20:07
@mikemhenry mikemhenry merged commit 15f20a6 into OpenFreeEnergy:main Sep 12, 2022
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.

3 participants