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

CLI component changes for new variant schema #100

Merged
merged 5 commits into from
Feb 17, 2014
Merged

CLI component changes for new variant schema #100

merged 5 commits into from
Feb 17, 2014

Conversation

nealsid
Copy link

@nealsid nealsid commented Feb 13, 2014

As in the other pull requests, this is a superset of the other commits and should decrease in size once those are merged. The commits meant to be reviewed in this PR are limited to the CLI changes for the new variant schema & changes.
Thanks!
Neal

@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/112/

* @return GATK Variant context post conversion.
*/
def convert(vc: ADAMVariantContext): VariantContext = {
val variant: ADAMVariant = ADAMVariant.newBuilder
Copy link
Member

Choose a reason for hiding this comment

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

The diff is a little screwy here—this is the body of the convert(VariantContext): ADAMVariantContext/ADAMVariant method, correct?

@fnothaft
Copy link
Member

The diff is actually really screwy on this one; is it OK for you if we wait on #97 and #98 to merge in before reviewing this?

@nealsid
Copy link
Author

nealsid commented Feb 13, 2014

Definitely.

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

The diff is actually really screwy on this one; is it OK for you if we
wait on #97 #97 and #98https://github.com/bigdatagenomics/adam/pull/98to merge in before reviewing this?


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

@@ -28,7 +28,9 @@ object AdamMain extends Logging {
PileupAggregator,
ListDict,
CompareAdam,
ComputeVariants,
/* TODO (nealsid): Reimplement in terms of new schema
Copy link
Member

Choose a reason for hiding this comment

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

Will this show up in a later PR?

Copy link
Author

Choose a reason for hiding this comment

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

Yes! Or I can remove it if it's going to be replaced by your variant
calling work in the future.

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

In adam-cli/src/main/scala/edu/berkeley/cs/amplab/adam/cli/AdamMain.scala:

@@ -28,7 +28,9 @@ object AdamMain extends Logging {
PileupAggregator,
ListDict,
CompareAdam,

  • ComputeVariants,
  • /* TODO (nealsid): Reimplement in terms of new schema

Will this show up in a later PR?


Reply to this email directly or view it on GitHubhttps://github.com//pull/100/files#r9764053
.

Copy link
Member

Choose a reason for hiding this comment

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

Nah! Just wanted to check.

Is it still necessary with the new schema? IIRC, in the current system, this is used for generating variants from genotypes; I'm not sure if this transformation makes sense with the new schema.

@massie
Copy link
Member

massie commented Feb 17, 2014

Jenkins, test this please.

@massie
Copy link
Member

massie commented Feb 17, 2014

@nealsid and @fnothaft -- looking over this pull request, it appears to be ready to merge into the VCF topic branch. Is that the case?

@fnothaft
Copy link
Member

It is OK from my end.

@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/126/

@massie
Copy link
Member

massie commented Feb 17, 2014

@nealsid If you confirm that the failing test is expected, I'll go ahead and merge this. Looking forward to your follow-on work.

@nealsid
Copy link
Author

nealsid commented Feb 17, 2014

@massie Thanks, these are expected.

@massie
Copy link
Member

massie commented Feb 17, 2014

Thanks, Neal!

massie added a commit that referenced this pull request Feb 17, 2014
CLI component changes for new variant schema
@massie massie merged commit d3570e8 into bigdatagenomics:vcf-work Feb 17, 2014
@nealsid nealsid deleted the vcf-work-variant-new-cli 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.

4 participants