Yf fix test removed deprecated methods relating to nm #728

Merged
merged 3 commits into from Oct 24, 2016

Conversation

Projects
None yet
3 participants
Contributor

yfarjoun commented Oct 21, 2016 edited

Description

Bug in SequenceUtil.calculateMdAndNmTags() whereby bases with different case (upper/lower) in reference or read are considered different despite the sam-spec saying that regarding SEQ: "No assumptions can be made on the letter cases." This has caused validatino problems for files that had their NM values set via SequenceUtil.calculateMdAndNmTags() (which takes case into account) and validated via SequenceUtil.calculateSamNmTag() (which, correctly, ignores case)

This PR fixes calculateMdAndNmTags by the same mechanism as used by calculateSamNmTag, adds a test to both functions (also testing MD at the same time) and gives variables in calculateMdAndNmTags more meaningful names.

Also, while I'm there I removed several deprecated functions that are not used.

I would like to nominate @jacarey for review as he seems to be the original author of calculateSamNmTag.

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)

yfarjoun added some commits Oct 21, 2016

@yfarjoun yfarjoun - tests added showing that the two codes for calculating NM do not ag…
…ree with each other (on lowercase bases in the reference and reads)
dd666b4
@yfarjoun yfarjoun - case-sensitivity of NM fixed. now tests pass. also refactored code …
…so that variables have meaningful names.
1c5354e

jacarey was assigned by yfarjoun Oct 21, 2016

Coverage Status

Coverage increased (+0.2%) to 69.264% when pulling 1c5354e on yf_fix_test_removed_deprecated_methods_relating_to_NM into 88b6719 on master.

@@ -985,66 +903,63 @@ public static void calculateMdAndNmTags(final SAMRecord record, final byte[] ref
final Cigar cigar = record.getCigar();
@jacarey

jacarey Oct 24, 2016

Contributor

May as well update the javadoc while we are in here.

Contributor

jacarey commented Oct 24, 2016

👍

@yfarjoun yfarjoun updated javadoc
4f1ca66
Contributor

yfarjoun commented Oct 24, 2016

waiting for tests to pass, then merging.

Coverage Status

Coverage increased (+0.2%) to 69.258% when pulling 4f1ca66 on yf_fix_test_removed_deprecated_methods_relating_to_NM into 88b6719 on master.

@yfarjoun yfarjoun merged commit 58f4154 into master Oct 24, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 69.258%
Details

yfarjoun deleted the yf_fix_test_removed_deprecated_methods_relating_to_NM branch Oct 24, 2016

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