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

Adding ADAMContig back to ADAMVariant. #245

Merged
merged 1 commit into from May 21, 2014

Conversation

Projects
None yet
4 participants
@fnothaft
Member

fnothaft commented May 21, 2014

This PR modifies some code that was changed in #238/#243. Specifically, we add the ADAMContig object back to the ADAMVariant, and we make several of the fields in the ADAMVariant/ADAMGenotype objects nullable.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 21, 2014

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/324/

AmplabJenkins commented May 21, 2014

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/324/

massie added a commit that referenced this pull request May 21, 2014

Merge pull request #245 from fnothaft/variant-avdl
Adding ADAMContig back to ADAMVariant.

@massie massie merged commit a7e71dc into bigdatagenomics:master May 21, 2014

1 check passed

default Merged build finished.
Details
@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie May 21, 2014

Member

Thanks, Frank!

Member

massie commented May 21, 2014

Thanks, Frank!

@hammer

This comment has been minimized.

Show comment
Hide comment
@hammer

hammer May 21, 2014

Contributor

For future reference, why was ADAMContig removed from ADAMVariant, and why is it okay to add it back?

Contributor

hammer commented May 21, 2014

For future reference, why was ADAMContig removed from ADAMVariant, and why is it okay to add it back?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 21, 2014

Member

@hammer Good question; I'm not 100% sure why it was removed (paging @mlinderm) but I believe the reason it was removed is because the code that converts from VCF only has (easy) access to the contig name.* I've added it back, because it is useful to track this info if we are generating VCFs from data that is already in ADAM (and that already has all the contig data). Otherwise, the contig name is a subset of the full contig info, so there's no correctness issue.

* I believe that the full contig info can be recovered from the VCF input; due to the way that Picard is designed, you need to do some dirty dancing. I'll try to fix this in a follow-on pull request.

Member

fnothaft commented May 21, 2014

@hammer Good question; I'm not 100% sure why it was removed (paging @mlinderm) but I believe the reason it was removed is because the code that converts from VCF only has (easy) access to the contig name.* I've added it back, because it is useful to track this info if we are generating VCFs from data that is already in ADAM (and that already has all the contig data). Otherwise, the contig name is a subset of the full contig info, so there's no correctness issue.

* I believe that the full contig info can be recovered from the VCF input; due to the way that Picard is designed, you need to do some dirty dancing. I'll try to fix this in a follow-on pull request.

@fnothaft fnothaft deleted the fnothaft:variant-avdl branch Jul 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment