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

Vcf work remove old variant #98

Merged
merged 6 commits into from
Feb 14, 2014
Merged

Vcf work remove old variant #98

merged 6 commits into from
Feb 14, 2014

Conversation

nealsid
Copy link

@nealsid nealsid commented Feb 13, 2014

This pull request is against vcf-work but it has commits that are inside vcf-work-schema (the other outstanding PR). Once vcf-work-schema is merged this PR will be smaller and only contained unreviewed changes. Let me know if there's a cleaner way to send this so only diffs between this and vcf-work-schema are shown.

@AmplabJenkins
Copy link

One or more automated tests failed
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/110/

@@ -30,521 +31,66 @@ import fi.tkk.ics.hadoop.bam.VariantContextWritable
* If an annotation has a corresponding set of fields in the VCF standard, a conversion to/from the
* GATK VariantContext should be implemented in this class.
*/
class VariantContextConverter extends Serializable {
private[adam] class VariantContextConverter extends Serializable with Logging {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE: making this class private to ADAM, I think that this is a good idea, but would note one downstream concern: it is useful to expose a method for converting an RDD of GATK VariantContexts to an RDD of AdamVariants. People who are building tools downstream may need to do this conversion—the big use case is if you have to open a pipe to/from an application that needs VCF. It'd probably be best to expose that via AdamRDDFunctions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done,

@fnothaft
Copy link
Member

Neal—do you want me to merge #97 to clean up these diffs?

@nealsid
Copy link
Author

nealsid commented Feb 13, 2014

Sure.

On Thu, Feb 13, 2014 at 1:10 PM, Frank Austin Nothaft <
notifications@github.com> wrote:

Neal—do you want me to merge #97https://github.com/bigdatagenomics/adam/pull/97to clean up these diffs?


Reply to this email directly or view it on GitHubhttps://github.com//pull/98#issuecomment-35007467
.

@fnothaft
Copy link
Member

@nealsid #97 is now merged; diff is much cleaner now!

@AmplabJenkins
Copy link

One or more automated tests failed
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/113/

@AmplabJenkins
Copy link

One or more automated tests failed
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/115/

@fnothaft
Copy link
Member

Thanks for the updates Neal! I am going to hold off on merging this PR through tomorrow, so that more people may review the code. If no more issues are raised by tomorrow afternoon, I will merge the request.

@nealsid
Copy link
Author

nealsid commented Feb 14, 2014

Thanks, Frank. Let me know if there's anything else blocking this.

@fnothaft
Copy link
Member

Looks good to me—will just wait until noon to merge in case anyone else wants to review the code; then I'll move on to #100 .

fnothaft added a commit that referenced this pull request Feb 14, 2014
@fnothaft fnothaft merged commit 549669c into bigdatagenomics:vcf-work Feb 14, 2014
@fnothaft
Copy link
Member

Thanks @nealsid ! I've just merged this PR.

@nealsid
Copy link
Author

nealsid commented Feb 14, 2014

Thanks, Frank!

On Fri, Feb 14, 2014 at 4:07 PM, Frank Austin Nothaft <
notifications@github.com> wrote:

Thanks @nealsid https://github.com/nealsid ! I've just merged this PR.


Reply to this email directly or view it on GitHubhttps://github.com//pull/98#issuecomment-35124945
.

@nealsid nealsid deleted the vcf-work-remove-old-variant branch February 20, 2014 15:53
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