-
Notifications
You must be signed in to change notification settings - Fork 308
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 ADAM to SAM conversion. #250
Conversation
All automated tests passed. |
} | ||
} | ||
|
||
//ERIC: Why does this take forever: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, sorry. Let me clean that...
All automated tests passed. |
.fold(dict)(dict + _) | ||
|
||
// build header | ||
val header: SAMFileHeader = createSAMHeader(refDict, readGroups) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we do this once per SAMRecord, rather than (first) collapsing all the ADAMRecords down into a SAMFileHeader and then passing that value in (calculated once) as an argument to this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, good point. I think we do things in backwards order here... I'll fix.
@tdanford I've just updated to generate the header before we do the record conversion. I have also rebased, because, y'know. |
All automated tests passed. |
All automated tests passed. |
* Converts a single ADAM record into a SAM record. | ||
* | ||
* @param adamRecord ADAM formatted alignment record to convert. | ||
* @param dict Reference sequence dictionary to use for conversion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line (and the next one) are out of date.
@fnothaft I just filed a PR to your feature branch, with (what I think is) a failing test... let me know if you think that's a valid test or not, though. |
Thanks @tdanford; I believe your failing test is a valid test, and I will review and fix. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/340/ |
Adding a round-trip test which fails (at the moment) with an NPE -- I *think* this is related to the failure to write out the RG tags?
@tdanford I've fixed the failing test, and have also cleaned up the other items you noted. |
All automated tests passed. |
|
||
import org.bdgenomics.adam.avro.ADAMRecord | ||
import org.bdgenomics.adam.models.{ | ||
Attribute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
} | ||
import org.bdgenomics.adam.rdd.ADAMContext._ | ||
import org.scalatest.FunSuite | ||
import scala.collection.JavaConverters._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
All automated tests passed. |
*/ | ||
package org.bdgenomics.adam.util | ||
|
||
import fi.tkk.ics.hadoop.bam.{ KeyIgnoringBAMOutputFormat, SAMFormat } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SAMFormat" is unused.
I'm going to just merge this as is and then followup with a pull request to clean up our import statements. |
Thanks, Frank! |
how to perform adam actions to convert from .adam file format to bam file. |
@ankushreddy you'll want to do:
|
This commit adds a conversion function for going from ADAM back to SAM or BAM. This work was done by @erictu; I've just gone ahead and rebased it on top of master, and done some small clean up. This resolves #49.