etal / cnvkit

Copy number variant detection from targeted DNA sequencing

http://cnvkit.readthedocs.org

Other

502 stars 163 forks source link

Extreme GC Fraction filter - mask_bad_bins - fix.py #752

Closed tsivaarumugam closed 1 year ago

tsivaarumugam commented 1 year ago

738 : Code change for accommodating skip extreme GC fraction option to switch on/off the filtering out of GC content below 0.3 & above 0.7

cnvkits_branch_0 9 8_function_mask_bad_bins (1) .

tsivaarumugam commented 1 year ago

@tetedange13, I have did the minor modifications to the code to accommodate your suggestion for issue #738 . Kindly review and let me know whether the changes can be merged to the master branch. - Thanks

tetedange13 commented 1 year ago

Hi @tsivaarumugam,

Thanks for your PR ! => I submitted a simpler one myself (#753) to adress this, with following differences :

No specific parameter to turn it on/off (both because it requires a lot of code changes + because not sure a lot of people need to disable this GC-content mask ?)
I only set a single MASK_GC value in params.py and always perform a symetric filtering [eg.: "0.3" filters "less than 0.3" and "above (1-0.3)=0.7"]

I suggest we let @etal decide which solution fits best its vision !

Hope this helps ! Have a nice day. Felix.

tsivaarumugam commented 1 year ago

@etal & @tetedange13 : Just for adding to comments about the Pull Request I have raised:

The code change has a command line option with store_false action, the GC filter will be off only if the parameter is mentioned in the command line as an argument flag. If this option is not mentioned as the command line parameter, the filter will be on (which is by default).
Also I have added the hard coded values of GC filter in the params.py code, so that users can specify the values whenever they need using the params.py.

Kindly go through the changes, if it is fine, kindly review the PR. - Thanks

etal commented 1 year ago

Hi @tsivaarumugam -- thanks for your patience. I'm reviewing this alongside #753, and I think I might need to combine the two to find a happy medium.

I also made a change related to #379 which was one of the reasons I added this filter originally; if the problem of residual GC bias is smaller now then there's less need for this filter.

tsivaarumugam commented 1 year ago

As the PR #753 addressing the same issue has been merged in to the master branch, I am closing this Pull Request. - Thanks