CNV consensus (4 of 6): Restructure column #349
Conversation
|
Fixing this right now |
There was a problem hiding this comment.
This looks good, assuming it passes CI this time.
I have a couple suggestions for simplifying, one in the snakefile and one in the script. The one in the script is more of a python tip than an important suggestion, so it is up to you if you want to do anything with it. I would probably make the Snakefile change though, as that substantially reduces redundancy.
| ## Initialize a variable to store the new restructred information | ||
| growing_list = '' | ||
|
|
||
| ## Rearange the split information into the new format | ||
| for j in range(0,len(list_start)): | ||
| growing_list += '{}:{}:{},'.format(list_start[j],list_end[j],list_cn[j]) |
There was a problem hiding this comment.
This is a great place for a python list comprehension, which is a bit faster (and more pythonic) than adding to a list bit by bit. It won't make much of a difference in this instance, but might be worth knowing about.
These lines could be replaced with:
cnv_list = ['{}:{}:{},'.format(*cnv) for cnv in zip(list_start, list_end, list_cn)]
There was a problem hiding this comment.
@jashapiro I see! Thank you for the suggestion. I never knew about the zip() function. It is helpful to know it exists! Thanks for that. I will add that in no problem!! Just a quick question, did you add an extra ] by accident at format(*cnv]) ?
There was a problem hiding this comment.
The * is not though, as you need to unpack the tuple.
There was a problem hiding this comment.
@jashapiro Yep! I fixed it and since list comprehension output my info in a list, I added join() to combine all of my strings together. Please have a look.
analyses/copy_number_consensus_call/src/scripts/restructure_column.py
Outdated
Show resolved
Hide resolved
Also minor format changes.
…lumn.py Co-Authored-By: jashapiro <jashapiro@gmail.com>
Purpose/implementation Section
Implement next step of copy number consensus call. This section reformats the start_pos, end_pos, and copy numbers of the individual CNVs so that these "raw" info can be retained even after multiple CNVs are merged together.
What GitHub issue does your pull request address?
issue #128
Is there anything that you want to discuss further?
Nope, this is a simple PR, it should be pretty quick to go through
Reproducibility Checklist