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

ADAM variant and genotype formats; and a VCF->ADAM converter #7

Closed
wants to merge 1 commit into from

Conversation

fnothaft
Copy link
Member

This is from the old repository, https://github.com/massie/adam/pull/12. This work was contributed by Chris Hartl as part of the work he did during his time at the AMPLab. He's unable to continue contributing work to this project as he's moved on to his next challenge (and will be missed).

This code is not fully clean nor complete yet, but I wanted to get it out into the open. There was a significant amount of discussion around this code when it originally hit the old repository, especially surrounding how to handle additional info fields. In the last PR, Chris had thrown some ideas out. We'd like to find someone to own this code moving forward — barring any volunteers, I will continue the cleanup efforts towards the end of this week.

@massie Matt, can you check the copyright info on the Vcf2Adam.scala file?

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@AmplabJenkins
Copy link

Merged build finished.

@AmplabJenkins
Copy link

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

@massie
Copy link
Member

massie commented Nov 25, 2013

Looks like this pull request isn't building. If you look at the console output of the build results, there's the following error:

[INFO] Compiling 36 Scala sources to /root/workspace/ADAM/adam-commands/target/scala-2.9.3/classes...
[ERROR] /root/workspace/ADAM/adam-commands/src/main/scala/edu/berkeley/cs/amplab/adam/commands/Reads2Ref.scala:109: value setReadName is not a member of edu.berkeley.cs.amplab.adam.avro.ADAMPileup.Builder
possible cause: maybe a semicolon is missing before `value setReadName'?
[ERROR]         .setReadName(record.getReadName)
[ERROR]          ^
[ERROR] one error found

@@ -0,0 +1,154 @@
/*
* Copyright (c) 2013. The Broad Institute of MIT/Harvard
Copy link
Member

Choose a reason for hiding this comment

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

This is the correct header. This work was done by Chris as an employee of The Broad.

@fnothaft
Copy link
Member Author

Looking into the build error now...

@fnothaft
Copy link
Member Author

Matt — this builds clean on my local machine. I believe this is a Jenkins issue: it looks like all of the last 3 builds have failed for this same reason. I've seen a similar issue before on my local. I believe that what is happening is that the autogenerated source inside of adam-format isn't getting rebuilt. Are we doing a full clean build on each pull request, or just an incremental build?

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@AmplabJenkins
Copy link

Merged build finished.

@AmplabJenkins
Copy link

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

@fnothaft
Copy link
Member Author

As an aside, I tried to merge all three pull requests from yesterday and ran across some interesting behavior. Specifically, when doing a merge, the following code gets dropped:

union { null, string } readName = null;
union { null, long } readStart = null;
union { null, long } readEnd = null;

This is in the *.avdl file.

I am not sure why this is happening, but once we merge the PRs in, we need to manually verify that those lines are still visible in the final merge, as otherwise the compile will fail.

@massie
Copy link
Member

massie commented Nov 25, 2013

You can also do a manual merge which gives you more control of the process.
There's a link near the merge button that give more details. You'll be able
to look at the exact change before pushing.

-Matt

On Mon, Nov 25, 2013 at 8:38 AM, Frank Austin Nothaft <
notifications@github.com> wrote:

As an aside, I tried to merge all three pull requests from yesterday and
ran across some interesting behavior. Specifically, when doing a merge, the
following code gets dropped:

union { null, string } readName = null;
union { null, long } readStart = null;
union { null, long } readEnd = null;

This is in the *.avdl file.

I am not sure why this is happening, but once we merge the PRs in, we need
to manually verify that those lines are still visible in the final merge,
as otherwise the compile will fail.


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

@laserson
Copy link
Contributor

In principle, I wouldn't mind taking over this part of the pipeline. One issue, however, is that I've never touched Scala. Perhaps this could be rewritten in Java?

@fnothaft
Copy link
Member Author

Practically, I think we'd rather keep as much code in Scala as possible, but we may be flexible. FWIW, there's a pretty low barrier to entry for Scala; neither Chris nor I had written in Scala before we started working on ADAM this fall. @massie is probably best to chime in here.

@tdanford
Copy link
Contributor

I'd vote for Scala -- but Uri, I'd be happy to help translate any Java prototypes you have into Scala if that would be useful to you. ADAM has been a great excuse, for me, to (re-)learn a lot of Scala.

@fnothaft
Copy link
Member Author

Ditto — I'd be glad to help with translation as well.

@massie
Copy link
Member

massie commented Nov 27, 2013

Manually merged. Thanks, Frank!

@massie massie closed this Nov 27, 2013
@fnothaft fnothaft deleted the merged-variant branch December 2, 2013 16:20
ryan-williams added a commit to ryan-williams/adam that referenced this pull request Feb 5, 2017
Merge in upstream, upgrade parent plugin
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.

None yet

5 participants