Expose the set of tags to reverse and reverse complement as parameters. #673

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Projects
None yet
4 participants
Owner

tfenne commented Aug 1, 2016

Description

Expose the set of tags to reverse and reverse complement as parameters, so that reads can be reverse complemented and all tags that are one-value-per-base in the read can be kept in sync.

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
  • Extended the README / documentation, if necessary
  • Is not backward compatible (breaks binary or source compatibility)

nh13 was assigned by tfenne Aug 1, 2016

@eitanbanks eitanbanks and 1 other commented on an outdated diff Aug 1, 2016

src/main/java/htsjdk/samtools/SAMRecordUtil.java
/**
* Reverse-complement all known sequence and base quality attributes of the SAMRecord.
*/
public static void reverseComplement(final SAMRecord rec) {
+ reverseComplement(rec, TAGS_TO_REVERSE_COMPLEMENT, TAGS_TO_REVERSE);
+ }
+
+ public static void reverseComplement(final SAMRecord rec, Collection<String> tagsToRevcomp, Collection<String> tagsToReverse) {
@eitanbanks

eitanbanks Aug 1, 2016

Contributor

Need 'final's here

@eitanbanks eitanbanks commented on the diff Aug 1, 2016

src/main/java/htsjdk/samtools/SAMRecordUtil.java
}
- for (final short stringTag : STRING_TAGS_TO_REVERSE) {
- final String value = (String)rec.getAttribute(stringTag);
- if (value != null) {
- rec.setAttribute(stringTag, StringUtil.reverseString(value));
+
+ // Deal with tags that needed to just be reversed
+ if (tagsToReverse != null) {
+ for (final String tag : tagsToReverse) {
+ Object value = rec.getAttribute(tag);
+ if (value != null) {
+ if (value instanceof String) {
+ value = StringUtil.reverseString((String) value);
+ }
+ else if (value.getClass().isArray()) {
@eitanbanks

eitanbanks Aug 1, 2016

Contributor

Just out of curiosity, why not maintain the previous behavior (converting to String and then reversing that)? Code was a lot cleaner. Is it b/c of bad performance?
Maybe pull all this out into a reverseArray() method that takes in an Object for code cleanliness?

@nh13

nh13 Aug 1, 2016

Contributor

It actually only supported String and not the array types. The increased complexity is to handle the various array types. I am not sure moving the 5 lines of code into a seperate reverseArray method is really cleaner.

Contributor

eitanbanks commented Aug 1, 2016

Comments made. @nh13 should feel free to comment himself (as the assigned reviewer) or just assign to me.

Contributor

nh13 commented Aug 1, 2016

@tfenne 👍 me but please respond to @eitanbanks comments.

tfenne was assigned by nh13 Aug 1, 2016

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 2, 2016

src/main/java/htsjdk/samtools/SAMRecordUtil.java
/**
* @author alecw@broadinstitute.org
*/
public class SAMRecordUtil {
-
- /** List of String tags that must be reversed if present when a SAMRecord is reverseComplemented */
- private static final short[] STRING_TAGS_TO_REVERSE = {
- SAMTagUtil.getSingleton().U2,
- SAMTagUtil.getSingleton().OQ
- };
+ private static List<String> TAGS_TO_REVERSE_COMPLEMENT = Arrays.asList(SAMTag.E2.name(), SAMTag.SQ.name());
+ private static List<String> TAGS_TO_REVERSE = Arrays.asList(SAMTag.OQ.name(), SAMTag.U2.name());
/**
* Reverse-complement all known sequence and base quality attributes of the SAMRecord.
@yfarjoun

yfarjoun Aug 2, 2016

Contributor

update javadoc

@yfarjoun yfarjoun commented on the diff Aug 2, 2016

src/main/java/htsjdk/samtools/SAMRecordUtil.java
+
+ // Deal with tags that needed to just be reversed
+ if (tagsToReverse != null) {
+ for (final String tag : tagsToReverse) {
+ Object value = rec.getAttribute(tag);
+ if (value != null) {
+ if (value instanceof String) {
+ value = StringUtil.reverseString((String) value);
+ }
+ else if (value.getClass().isArray()) {
+ if (value instanceof byte[]) reverseArray((byte[]) value);
+ else if (value instanceof short[]) reverseArray((short[]) value);
+ else if (value instanceof int[]) reverseArray((int[]) value);
+ else if (value instanceof float[]) reverseArray((float[]) value);
+ else throw new UnsupportedOperationException("Reversing array attribute of type " + value.getClass().getComponentType() + " not supported.");
+ }
@yfarjoun

yfarjoun Aug 2, 2016

Contributor

shouldn't there be another else clause here?

@tfenne

tfenne Aug 2, 2016

Owner

Yes, done.

@tfenne tfenne Expose the set of tags to reverse and reverse complement as parameters.
5099e10
Owner

tfenne commented Aug 2, 2016

Looks like just the remote tests are failing (again), and nothing to do with this PR. I've re-kicked the build, but if they fail again I'll merge anyway.

@tfenne tfenne merged commit ec2d3f9 into master Aug 2, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

tfenne deleted the tf_rc_samrecord_params branch Aug 2, 2016

@jamesemery jamesemery added a commit to jamesemery/htsjdk that referenced this pull request Sep 1, 2016

@tfenne @jamesemery tfenne + jamesemery Expose the set of tags to reverse and reverse complement as parameter…
…s. (#673)
2da42f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment