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

Modifying conversion code to resolve #112. #260

Merged
merged 1 commit into from
Jun 9, 2014

Conversation

fnothaft
Copy link
Member

@fnothaft fnothaft commented Jun 7, 2014

Resolves #112. Specifically:

  • Adds code to SAM->ADAM conversion to only set mapping flags if the record is mapped.
  • Adds fields to the ADAMRecord to disambiguate secondary vs. supplementary alignments
  • Adds accompanying code to the ADAM->SAM conversion to ensure that secondary vs. supplementary alignments are properly converted back to SAM.
  • Updates DecadentRead to add the requirement statement that was removed because of SAMRecordConverter: ensure internal consistency of flags #112.

@AmplabJenkins
Copy link

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

@tdanford
Copy link
Contributor

tdanford commented Jun 8, 2014

I merged another of your PR's, so don't forget to rebase this one!

@fnothaft
Copy link
Member Author

fnothaft commented Jun 8, 2014

@tdanford rebased!

@AmplabJenkins
Copy link

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

// - secondary is a "less good" linear alignment
// - supplementary is a non-primary linear alignment in a chimeric alignment
union { boolean, null } primaryAlignment = false;
union { boolean, null } secondaryAlignment = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I read the code above correctly, your new "secondaryAlignment" flag is equivalent to "mapped, but not primary or supplementary." Is that right? If so, can you clarify the description in the comment block above?

Also, is there any interaction here with the code in ReadBucket and SingleReadBucket? In particular, there are already fields in those classes with particles like "secondary" in their names -- do they match the use of this "secondaryAlignment" flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the comment. I'm not going to touch the ReadBucket/SingleReadBucket code. I think primary/secondary there maps to primary/(secondary || supplemental), but it's not clear. @massie, can you clarify?

@AmplabJenkins
Copy link

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

tdanford added a commit that referenced this pull request Jun 9, 2014
Modifying conversion code to resolve #112.
@tdanford tdanford merged commit 596eae3 into bigdatagenomics:master Jun 9, 2014
@tdanford
Copy link
Contributor

tdanford commented Jun 9, 2014

Thanks, Frank!

@fnothaft fnothaft deleted the sanity-check-flags branch July 10, 2014 13:15
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.

SAMRecordConverter: ensure internal consistency of flags
3 participants