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

Fix Adam2fastq in case of read with both reverse and unmapped flags #982

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jpdna
Member

jpdna commented Mar 26, 2016

Fixes the bug in fastq dump discussed in #980
specifically for case of a read that has both reverse and unmapped flags set in the input bam file.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 26, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1122/
Test PASSed.

AmplabJenkins commented Mar 26, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1122/
Test PASSed.

Show outdated Hide outdated ...scala/org/bdgenomics/adam/converters/AlignmentRecordConverterSuite.scala
@@ -222,7 +222,7 @@ class AlignmentRecordConverterSuite extends FunSuite {
}
test("converting to fastq with unmapped reads") {
test("converting to fastq with unmapped reads when read reverse strand flag was set") {

This comment has been minimized.

@heuermh

heuermh Mar 28, 2016

Member

could you create a new unit test instead of modifying this existing one? it is better to always be increasing unit test coverage

@heuermh

heuermh Mar 28, 2016

Member

could you create a new unit test instead of modifying this existing one? it is better to always be increasing unit test coverage

This comment has been minimized.

@jpdna

jpdna Mar 29, 2016

Member

The test as it is was previously written handled the case which it was then testing, which was the case of reverse stand flag with negative flag, incorrectly and thus failed with the changes introduced by this PR. I modified the test to be correct, and the change in the message above to converting to fastq with unmapped reads when read reverse strand flag was set just describes the test more specifically.

@jpdna

jpdna Mar 29, 2016

Member

The test as it is was previously written handled the case which it was then testing, which was the case of reverse stand flag with negative flag, incorrectly and thus failed with the changes introduced by this PR. I modified the test to be correct, and the change in the message above to converting to fastq with unmapped reads when read reverse strand flag was set just describes the test more specifically.

This comment has been minimized.

@fnothaft

fnothaft Mar 29, 2016

Member

I would agree with @heuermh; specifically, I would fix the current test, and add a new test that tests the "unmapped + reverse strand flag" case.

@fnothaft

fnothaft Mar 29, 2016

Member

I would agree with @heuermh; specifically, I would fix the current test, and add a new test that tests the "unmapped + reverse strand flag" case.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 29, 2016

Member

Other than the test case nit, LGTM!

Member

fnothaft commented Mar 29, 2016

Other than the test case nit, LGTM!

Added test to include fastq generation of unampped reads both with an…
…d without rev complement (0x10) set. Also revised 'first' and 'second' ordering in tests to me more clear.
@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Mar 29, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1127/
Test PASSed.

AmplabJenkins commented Mar 29, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1127/
Test PASSed.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 29, 2016

Member

Thanks for the test fix! Can you squash down and rebase? @heuermh if it looks good to you, I will merge.

Member

fnothaft commented Mar 29, 2016

Thanks for the test fix! Can you squash down and rebase? @heuermh if it looks good to you, I will merge.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Mar 29, 2016

Member

+1

Member

heuermh commented Mar 29, 2016

+1

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna Mar 29, 2016

Member

I updated PR so that tests cover correctly both umapped with Ox10 set and unset. The existing test did need to be revised as it was incorrect before. I had to add a the new test using different different records because actually the case which was not covered previously was the case of umapped without Ox10 set, and required a different record to test.

I also revised to be consistent what was previously a confusing re-ordering of (firstRecord,SecondRecord) here when return tuple values are assigned in these tests. As far as I can tell first and second here only refer to the order in which they appear in the input SAM file, and the assertion of

assert(firstRecord.getReadInFragment === 1)
assert(secondRecord.getReadInFragment === 0)

or

assert(firstRecord.getReadInFragment === 0)
assert(secondRecord.getReadInFragment === 1)

is more clear if the tuple order remains the same and the val names are not switched when parsing the tuple.

And yes - I'll squash and rebase

Member

jpdna commented Mar 29, 2016

I updated PR so that tests cover correctly both umapped with Ox10 set and unset. The existing test did need to be revised as it was incorrect before. I had to add a the new test using different different records because actually the case which was not covered previously was the case of umapped without Ox10 set, and required a different record to test.

I also revised to be consistent what was previously a confusing re-ordering of (firstRecord,SecondRecord) here when return tuple values are assigned in these tests. As far as I can tell first and second here only refer to the order in which they appear in the input SAM file, and the assertion of

assert(firstRecord.getReadInFragment === 1)
assert(secondRecord.getReadInFragment === 0)

or

assert(firstRecord.getReadInFragment === 0)
assert(secondRecord.getReadInFragment === 1)

is more clear if the tuple order remains the same and the val names are not switched when parsing the tuple.

And yes - I'll squash and rebase

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 29, 2016

Member

Actually, I can take care of the rebase + squash when merging. I'll merge this now...

Member

fnothaft commented Mar 29, 2016

Actually, I can take care of the rebase + squash when merging. I'll merge this now...

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 29, 2016

Member

Thanks @jpdna! Squashed and merged as b8e36b2.

Member

fnothaft commented Mar 29, 2016

Thanks @jpdna! Squashed and merged as b8e36b2.

@fnothaft fnothaft closed this Mar 29, 2016

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