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

Improvements to file, tempdir, and command handling #41

Closed

Conversation

DarwinAwardWinner
Copy link
Contributor

I've made a number of improvements to file handling, tempdir handling, and command handling. In the files I edited:

  • Temporary directory handling is delegated to the tempfile module.
  • subprocess.check_call is used instead of subprocess.call, so that if a command fails, the program will abort.
  • bedgraph paths are converted into bigwig paths via os.path functions instead of string replacement
  • os.makedirs is used in place of call("mkdir -p").

My original motivation for making these fixes was noticing that every invocation of epic used the same temporary directory for bedgraph files, which meant that if multiple instances of the script were running at once, it was possible that they could overwrite each others' temporary files, causing bigwig files to get associated with the wrong samples. This should no longer be an issue, since each invoocation creates its own randomly-named temporary directory.

In matrixes.py:

- Compute bigwig paths from bedgraph using os.path functions
- Replace 'call("mkdir -p {}")' with 'os.makedirs'
- Replace 'call(shell=True)' with 'check_call(shell=False)'
- Remove obsolete tmpdir handling code
@coveralls
Copy link

coveralls commented Oct 1, 2016

Coverage Status

Coverage decreased (-0.002%) to 64.787% when pulling 209a57f on DarwinAwardWinner:bdg-tmpdir into a638713 on endrebak:master.

@coveralls
Copy link

coveralls commented Oct 1, 2016

Coverage Status

Coverage decreased (-1.7%) to 63.049% when pulling 25b22c1 on DarwinAwardWinner:bdg-tmpdir into a638713 on endrebak:master.

@endrebak
Copy link
Member

endrebak commented Oct 3, 2016

Thanks, much appreciated, but I have been rewriting the bigwig code to use rtracklayer (from bioconda-bioconductor) so this might be superfluous :/

This isn't your typical bioinformatics package that does not get updated, so before PRs you might want to make an issue first :)

@endrebak
Copy link
Member

endrebak commented Oct 3, 2016

Thanks for this, but in 0.1.19 I've improved the handling of bigwigs. Furthermore, your tests failed for 2.7 :)

@endrebak endrebak closed this Oct 3, 2016
@DarwinAwardWinner
Copy link
Contributor Author

That's ok, I'm a heavy Bioconductor user, so I support that solution. But I see that your changes also removed the option for outputting single bigwigs for the sum of all inputs and all treatments. Is there a reason for that? I've been using that option in my workflow, and I'd like to know if there's a reason I shouldn't be using it.

@endrebak
Copy link
Member

endrebak commented Oct 4, 2016

Sorry, I had no idea. I removed it because I thought I wasn't going to use
it and that it was such a new feature that no-one else had either :)

Since the new code is much shorter and faster, I'll find a way to create
the sums of bigwigs using it. For now, please use 0.1.18 for the time
being.

On Mon, Oct 3, 2016 at 8:25 PM, Ryan C. Thompson notifications@github.com
wrote:

That's ok, I'm a heavy Bioconductor user, so I support that solution. But
I see that your changes also removed the option for outputting single
bigwigs for the sum of all inputs and all treatments. Is there a reason for
that? I've been using that option in my workflow, and I'd like to know if
there's a reason I shouldn't be using it.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#41 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AQ9I0vvq_c0VLCn0N75MpTChQEKd93pvks5qwUibgaJpZM4KLnuS
.

@endrebak
Copy link
Member

endrebak commented Oct 4, 2016

pip install epic=0.1.20 should write sum of bigwigs. Not out in bioconda yet.

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