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

Replace plotnine implementation of barplot with seaborn/matplotlib #441

Merged
merged 11 commits into from
Nov 20, 2023
Merged

Replace plotnine implementation of barplot with seaborn/matplotlib #441

merged 11 commits into from
Nov 20, 2023

Conversation

kcArtemis
Copy link
Contributor

Removed the dependency (plotnine)

PR Checklist

  • Referenced issue is linked
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

In this pull request, I addressed the issue #425 by removing the dependency on plotnine. Instead, I have implemented the use of matplotlib and seaborn for plotting. Specifically, I updated the barplot function and the perturbscore function to utilize these libraries, providing a more lightweight solution for the project.

Technical details

While making these changes, I ensured that the functionality remains consistent, and the transition to matplotlib and seaborn is seamless. The choice of these libraries was made to minimize dependencies and provide a more straightforward solution for users. No additional dependencies are introduced, and the overall performance is maintained.

Additional context

kcArtemis and others added 2 commits November 18, 2023 10:55
Removed the dependency (plotnine)
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Merging #441 (7a1edfe) into main (e17ade7) will decrease coverage by 0.41%.
The diff coverage is 1.81%.

❗ Current head 7a1edfe differs from pull request most recent head 79f4d20. Consider uploading reports for the commit 79f4d20 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   58.23%   57.83%   -0.41%     
==========================================
  Files          40       40              
  Lines        4899     4933      +34     
==========================================
  Hits         2853     2853              
- Misses       2046     2080      +34     
Files Coverage Δ
pertpy/plot/_mixscape.py 9.61% <1.81%> (-1.88%) ⬇️

@Zethson Zethson changed the title Update _mixscape.py Replace plotnine implementation of barplot with seaborn/matplotlib Nov 20, 2023
Copy link
Member

@Zethson Zethson left a comment

Choose a reason for hiding this comment

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

Great!
Thank you very much!

  1. Could you please remove plotnine from the dependencies from pyproject.toml?
  2. Could you please paste a before and after plot here? Like one with the old plotnine implementation and one with your new implementation
  3. We have a lot of hardcoded plot parameters now (like s, alpha,, and more). Which ones do you think we should expose to the user? Should we populate them at least with kwargs?

Thanks!

pertpy/plot/_mixscape.py Outdated Show resolved Hide resolved
pertpy/plot/_mixscape.py Outdated Show resolved Hide resolved
pertpy/plot/_mixscape.py Outdated Show resolved Hide resolved
Have removed the commented sentence at line 112 and have changed the variable name from "I" to "cond" (meaning condition)
I have removed the name of the dependency (plotnine)
@kcArtemis
Copy link
Contributor Author

Great! Thank you very much!

  1. Could you please remove plotnine from the dependencies from pyproject.toml?
  2. Could you please paste a before and after plot here? Like one with the old plotnine implementation and one with your new implementation
  3. We have a lot of hardcoded plot parameters now (like s, alpha,, and more). Which ones do you think we should expose to the user? Should we populate them at least with kwargs?

Thanks!

we can expose s and alpha to the user since s:marker size and alpha: transparency parameter, for that we can make the use of kwargs.

@Zethson
Copy link
Member

Zethson commented Nov 20, 2023

images_pertpy_mixscale.pdf

Before and after by @kcArtemis

pertpy/plot/_mixscape.py Outdated Show resolved Hide resolved
@Zethson Zethson linked an issue Nov 20, 2023 that may be closed by this pull request
6 tasks
@subwaystation

This comment was marked as off-topic.

Updated "p" variable to "plot_dens" and change the legend title from "mix" to "mix scape_class"
@Zethson Zethson merged commit 09f7d73 into scverse:main Nov 20, 2023
4 of 7 checks passed
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.

Minimize dependencies
3 participants