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 more granular variant specification and other minor changes #165

Merged
merged 5 commits into from
Dec 17, 2016

Conversation

tavinathanson
Copy link
Member

Added a few things to support some of my melanoma and lung work.

In this PR:

  • Biggest change is removing snv_vcf_paths and indel_vcf_paths and replacing with a dictionary from snv to a list of VCF paths or MAF paths or VariantCollections.
  • Allow filtering of a Cohort.
  • Allow users to grab all effects, not just top priority ones.
  • Add a .genome property.

Since removing snv_vcf_paths will break things, I'd be bumping the release up to 0.5.0. Also need to do more manual/automated testing to confirm no issues.

@jburos
Copy link
Member

jburos commented Dec 16, 2016

First, thanks for working on this & helping to tackle these issues!

Few high-level comments:

  1. Not sure I like the dict approach to indel, snv or any. Why not include all VCF/MAFs irrespective of type & handle the filtering by type in a function? (e.g. have variant_count, snv_count, indel_count, etc)?
  2. Also, not sure I like breaking backward compatibility for this reason. I think we can add an option for vcf_paths (to accept a list) and populate this with values from snv_vcf_paths & indel_vcf_paths if the user passes these in?
  3. I do think the behavior of variant-type default behavior is a little tricky to reconcile without modifying the default filter fn. How wedded to this are you?

I would like to test this out if that's OK with you & will then give more concrete feedback.

@coveralls
Copy link

coveralls commented Dec 16, 2016

Coverage Status

Coverage decreased (-0.3%) to 55.477% when pulling 4b77794 on variant_import into 2a306b8 on master.

@jburos
Copy link
Member

jburos commented Dec 16, 2016

Just noting here our offline discussion:

  1. remove dict of vcf files; replace with list.
  2. remove variant_type param; filter types using summary functions.
  3. OK to break backwards compatibility.

Otherwise LGTM --

@coveralls
Copy link

coveralls commented Dec 17, 2016

Coverage Status

Coverage increased (+0.02%) to 55.79% when pulling 34f10f0 on variant_import into 2a306b8 on master.

@tavinathanson
Copy link
Member Author

@jburos responded to all the above, and added an associated test as well as an error if a cache with the old format is present. I don't want to bother you over the weekend, so I'll probably go ahead and merge today or tomorrow.

@coveralls
Copy link

coveralls commented Dec 17, 2016

Coverage Status

Coverage increased (+0.02%) to 55.79% when pulling a02961b on variant_import into 2a306b8 on master.

@jburos
Copy link
Member

jburos commented Dec 17, 2016

LGTM - thanks Tavi! Really like the out-of-date cache check.

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