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

Add a parameter to name kink image in thresh_peaks #44

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

julianstanley
Copy link
Collaborator

@julianstanley julianstanley commented Jun 18, 2022

Explain your changes:

I am using this pipeline on real data and am calculating Z scores for multiple files, like this:

image

There's two issues here: (1) it combines the kink plots from all the files, so I just added plt.figure() to my wrapper,

image

and (2) it saves all the images as kink.png, so it overwrites previous ones.

No problem, let's just add a parameter to thresh_peaks to let us name the image files so I can save them each with the name of their associated wig file.

Does this close any issues?

Any comments? Anything in particular you need feedback on?

PR checklist

  • I have written tests for my changes where needed and made sure they pass locally.

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2022

Codecov Report

Merging #44 (0eba2d2) into main (727fada) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #44   +/-   ##
=======================================
  Coverage   77.68%   77.68%           
=======================================
  Files           8        8           
  Lines         802      802           
=======================================
  Hits          623      623           
  Misses        179      179           
Flag Coverage Δ
unittests 77.68% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rendseq/make_peaks.py 99.07% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@julianstanley
Copy link
Collaborator Author

It's a shame that matplotlib doesn't make it easy to save plots as variables, like in R/ggplot. That would make it easier for that plot to just be an additional return

@miraep8
Copy link
Collaborator

miraep8 commented Jun 21, 2022

This sounds very reasonable haha. As a sidenote, I think we might actually be able to return the plot by doing something like this actually (though this might not be what you had in mind):

import numpy as np
from matplotlib import pyplot as plt

def gen_plot():
    my_fig = plt.figure()
    plt.scatter([i for i in range(20)], [i for i in range(20)])
    return my_fig

f = gen_plot()

plt.show(f)

Just request me for a review when you are ready to merge! :)

@miraep8
Copy link
Collaborator

miraep8 commented Feb 24, 2023

By the way, just revisiting this very reasonable PR. Any reason not to merge? I think I was originally holding off because it was a draft PR (so I wasn't sure if there was more to it) but I am happy with it as is! :)

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