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-785] Add support for all numeric array (TYPE=B) tags, fixes #785 #809

Closed
wants to merge 2 commits into from

Conversation

jfeala
Copy link

@jfeala jfeala commented Aug 26, 2015

Please review carefully, I'm a Scala beginner! Advice welcome for better, more Scala-ish patterns

  • Extends Attribute model and AttributeUtils.convertSAMTagAndValue to cover the 7 numeric array types [cCsSiIf] described in SAM spec
  • Updates tests to use TextTagCodec used natively by samtools parser
  • Adds a small SAM file with all tag types and tests with loadAlignments

- Extends `Attribute` model and `AttributeUtils.convertSAMTagAndValue` to cover the 7 numeric array types [cCsSiIf] described in SAM spec
- Updates tests to use `TextTagCodec` used natively by samtools parser
- Adds a small SAM file with all tag types and tests with loadAlignments
@AmplabJenkins
Copy link

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

val tagArrayString = attrTuple._3
val valueStr = attrTuple._4

val fullTagString = if (tagArrayString == null) tagTypeString else
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking for a null String here, I would probably make tagArrayString be Option[String] and then change this line to be a fold:

val fullTagString = tagArrayString.fold(tagTypeString)(arrayString => "%s:%s".format(tagTypeString, arrayString))

@fnothaft
Copy link
Member

Generally looks good to me, other than the small nit above. Thanks for the contribution @jfeala!

@massie
Copy link
Member

massie commented Aug 27, 2015

Looks good to me, too. Thanks for the contribution, Jake. Appreciated.

@jfeala
Copy link
Author

jfeala commented Aug 27, 2015

Thanks, guys. @fnothaft I updated that line, thanks for the suggestion.

Let me know if you want me to squash the extra commit before merge

@AmplabJenkins
Copy link

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

@fnothaft
Copy link
Member

Ping for anyone else who wants to review; if no one else wants to review, I will merge this noon PST tomorrow.

@jfeala no worries on squashing; I can take care of that when merging.

@massie
Copy link
Member

massie commented Aug 28, 2015

👍

@massie
Copy link
Member

massie commented Sep 9, 2015

Thanks, @jfeala !

@massie
Copy link
Member

massie commented Sep 9, 2015

Merged manually after rebase.

@massie massie closed this Sep 9, 2015
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