Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag). #636

nh13 · 2016-08-20T01:50:23Z

Rename SetNmAndUqTags to SetNmMDAndUqTags.
WIP
Fixes: Update the NM and MD tags when clipping overlapping inserts in #622

Fixes #634 as an alternate to reverting (#635).

coveralls · 2016-08-20T02:21:19Z

Coverage decreased (-0.02%) to 69.24% when pulling 9d2018a on nh_update_md_and_nm into f4fcb3e on master.

yfarjoun · 2016-08-20T02:28:52Z

src/main/java/picard/sam/AbstractAlignmentMerger.java

-        final boolean calculateMd = rec.getStringAttribute(SAMTag.MD.name()) != null;
-        if (calculateNm || calculateMd) {
-            final byte[] referenceBases = this.refSeq.get(this.refSeq.getSequenceDictionary().getSequenceIndex(rec.getReferenceName())).getBases();
-            SequenceUtil.calculateMdAndNmTags(rec, referenceBases, calculateNm, calculateMd);


in this code that you removed calculateNm, calculateMd were reversed!

I see it. I really like scala where we can have named params. Sigh.

I agree. especially important for booleans. but all params could benefit. not sure why java don't have it too....

coveralls · 2016-08-20T02:33:37Z

Coverage increased (+0.03%) to 69.289% when pulling 6463c53 on nh_update_md_and_nm into f4fcb3e on master.

yfarjoun · 2016-08-20T02:37:00Z

if I understand correctly, this reverts the fixing of tags when the cigar changes, but includes MD in the fixTags CLP. Should we purge MD NM and UQ when we change the cigar? At-least we will not be leaving around incorrect information....

nh13 · 2016-08-20T02:43:29Z

@yfarjoun: If downstream users are impacted (i.e. they want MD tags but no longer have them), then they have been relying on potentially the wrong values. I vote for removing NM/MD/UQ when we clip overlapping reads.

Also, should we do the same for clipping adapters (see updateCigarForTrimmedOrClippedBases and this.clipAdapters)?

tfenne · 2016-08-20T09:23:00Z

src/main/java/picard/sam/SetNmMDAndUqTags.java

-public class SetNmAndUqTags extends CommandLineProgram {
-    static final String USAGE_SUMMARY = "Fixes the UQ and NM tags in a SAM file.  ";
-    static final String USAGE_DETAILS = "This tool takes in a SAM or BAM file (sorted by coordinate) and calculates the NM and UQ tags by comparing with the reference."+
+public class SetNmMDAndUqTags extends CommandLineProgram {


It's going to drive me crazy that it's Nm, Uq and MD in the name. Can you change to Md please?

tfenne · 2016-08-20T09:24:39Z

@nh13 @yfarjoun Agreed that MergeBamAlignment should probably remove tags rather than output invalid values. The other option is to add back @nh13's change but have it use either the indexed fasta file (potentially loading the same region many times) or load the entire reference into memory (requiring more memory).

yfarjoun · 2016-08-20T16:12:03Z

that would require a new type of ReferenceFile reader, no?

On Sat, Aug 20, 2016 at 5:24 AM, Tim Fennell notifications@github.com
wrote:

@nh13 https://github.com/nh13 @yfarjoun https://github.com/yfarjoun
Agreed that MergeBamAlignment should probably remove tags rather than
output invalid values. The other option is to add back @nh13
https://github.com/nh13's change but have it use either the indexed
fasta file (potentially loading the same region many times) or load the
entire reference into memory (requiring more memory).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#636 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACnk0ptUenR033on5rmpL9wnQw8ju-nWks5qhsfZgaJpZM4Jo-Dm
.

nh13 · 2016-08-23T18:34:00Z

@yfarjoun I think I will remove the tags, and in the case folks output queryname, the old behavior stands. I will add some documentation somewhere about this too.

yfarjoun · 2016-08-24T00:13:05Z

but even if outputting in queryname the tags will be incorrect...so why not
remove them from the reads that are clipped?

On Tue, Aug 23, 2016 at 2:34 PM, Nils Homer notifications@github.com
wrote:

@yfarjoun https://github.com/yfarjoun I think I will remove the tags,
and in the case folks output queryname, the old behavior stands. I will add
some documentation somewhere about this too.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#636 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACnk0kp__OdulpB9mvhUqpjn68QZJkFzks5qiz0agaJpZM4Jo-Dm
.

nh13 · 2016-08-26T01:14:53Z

@yfarjoun see my latest changes. I remove the NM/MD/UQ tags for reads that are clipped due to adapters or overlapping bases. I also added some doc to MergeBamAlignment.

yfarjoun · 2016-08-28T12:56:26Z

src/main/java/picard/sam/AbstractAlignmentMerger.java

@@ -477,7 +476,7 @@ public void mergeAlignment(final File referenceFasta) {

            for (final SAMRecord rec : sink.sorter) {
                if (!rec.getReadUnmappedFlag() && refSeq != null) {
-                    fixNMandUQ(rec, refSeq, bisulfiteSequence);
+                    fixNmMdAndUQ(rec, refSeq, bisulfiteSequence);


yfarjoun · 2016-08-28T13:02:50Z

can I haz A small test that shows that the tags are actually removed?

yfarjoun · 2016-08-28T13:03:01Z

back to you @nh13

yfarjoun · 2016-08-29T15:55:40Z

why assigning me here? this has some work to be done:

tests are not passing
need to add a small test
add a deprecated wrapper with the old name?

Thanks! :-)

nh13 · 2016-08-29T16:16:02Z

@yfarjoun you need to un-assign yourself even if you assign someone else. Multiple folks can be assigned an issue now.

nh13 · 2016-09-06T01:20:45Z

@yfarjoun added the test and rebased onto master. After a final 👍 this is ready to go.

coveralls · 2016-09-06T01:28:19Z

Coverage increased (+0.05%) to 69.394% when pulling 57c0ac1 on nh_update_md_and_nm into 302da16 on master.

yfarjoun · 2016-09-18T19:09:23Z

src/main/java/picard/sam/SetNmMdAndUqTags.java

+
+    private final Log log = Log.getInstance(SetNmMdAndUqTags.class);
+
+    public static void main(final String[] argv) {


is this main still needed with the new system?

Not sure, but I am going to leave it here as other classes have it; cleaning this up should be in all classes could be in another PR.

yfarjoun · 2016-09-18T19:10:22Z

src/main/java/picard/sam/SetNmMdAndUqTags.java

+
+        final ReferenceSequenceFileWalker refSeq = new ReferenceSequenceFileWalker(REFERENCE_SEQUENCE);
+
+       StreamSupport.stream(reader.spliterator(),false)


extra space missing here?

aligning the line, and after comma.

yfarjoun · 2016-09-18T19:20:34Z

sorry for the delay. thanks!! 👍

1. Rename SetNmAndUqTags to SetNmMDAndUqTags. 2. Fixes: #622

coveralls · 2016-09-19T02:34:19Z

Coverage increased (+0.04%) to 70.007% when pulling 0ea9d4c on nh_update_md_and_nm into e7d6c8f on master.

nh13 mentioned this pull request Aug 20, 2016

Exception running MergeBamAlignment after #622 #634

Closed

nh13 force-pushed the nh_update_md_and_nm branch from 505970d to 9d2018a Compare August 20, 2016 02:12

nh13 force-pushed the nh_update_md_and_nm branch from 9d2018a to 6463c53 Compare August 20, 2016 02:24

yfarjoun reviewed Aug 20, 2016
View reviewed changes

tfenne reviewed Aug 20, 2016
View reviewed changes

nh13 self-assigned this Aug 23, 2016

nh13 force-pushed the nh_update_md_and_nm branch from 6463c53 to 8728570 Compare August 26, 2016 01:14

nh13 assigned yfarjoun and unassigned nh13 Aug 26, 2016

yfarjoun reviewed Aug 28, 2016
View reviewed changes

yfarjoun assigned nh13 Aug 28, 2016

yfarjoun removed their assignment Aug 29, 2016

yfarjoun mentioned this pull request Sep 1, 2016

Revert "Update the NM and MD tags when clipping overlapping inserts in" #635

Merged

nh13 force-pushed the nh_update_md_and_nm branch from 1c24dfb to 57c0ac1 Compare September 6, 2016 01:20

nh13 assigned yfarjoun and unassigned nh13 Sep 6, 2016

yfarjoun reviewed Sep 18, 2016

View reviewed changes

Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag).

0ea9d4c

1. Rename SetNmAndUqTags to SetNmMDAndUqTags. 2. Fixes: #622

nh13 force-pushed the nh_update_md_and_nm branch from 57c0ac1 to 0ea9d4c Compare September 19, 2016 02:25

nh13 merged commit 915ffa7 into master Sep 19, 2016

nh13 deleted the nh_update_md_and_nm branch September 19, 2016 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag). #636

Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag). #636

nh13 commented Aug 20, 2016

coveralls commented Aug 20, 2016

yfarjoun Aug 20, 2016

nh13 Aug 20, 2016

yfarjoun Aug 20, 2016

coveralls commented Aug 20, 2016

yfarjoun commented Aug 20, 2016

nh13 commented Aug 20, 2016

tfenne Aug 20, 2016

tfenne commented Aug 20, 2016

yfarjoun commented Aug 20, 2016

nh13 commented Aug 23, 2016

yfarjoun commented Aug 24, 2016

nh13 commented Aug 26, 2016

yfarjoun Aug 28, 2016

nh13 Sep 6, 2016

yfarjoun commented Aug 28, 2016

yfarjoun commented Aug 28, 2016

yfarjoun commented Aug 29, 2016

nh13 commented Aug 29, 2016

nh13 commented Sep 6, 2016

coveralls commented Sep 6, 2016

yfarjoun Sep 18, 2016

nh13 Sep 19, 2016

yfarjoun Sep 18, 2016

yfarjoun Sep 18, 2016

nh13 Sep 19, 2016

yfarjoun commented Sep 18, 2016

coveralls commented Sep 19, 2016


		private final Log log = Log.getInstance(SetNmMdAndUqTags.class);

		public static void main(final String[] argv) {


		final ReferenceSequenceFileWalker refSeq = new ReferenceSequenceFileWalker(REFERENCE_SEQUENCE);

		StreamSupport.stream(reader.spliterator(),false)

Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag). #636

Rename SetNmAndUqTags and fix #622 (MergeBamAlignment MD tag). #636

Conversation

nh13 commented Aug 20, 2016

coveralls commented Aug 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 20, 2016

yfarjoun commented Aug 20, 2016

nh13 commented Aug 20, 2016

Choose a reason for hiding this comment

tfenne commented Aug 20, 2016

yfarjoun commented Aug 20, 2016

nh13 commented Aug 23, 2016

yfarjoun commented Aug 24, 2016

nh13 commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Aug 28, 2016

yfarjoun commented Aug 28, 2016

yfarjoun commented Aug 29, 2016

nh13 commented Aug 29, 2016

nh13 commented Sep 6, 2016

coveralls commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Sep 18, 2016

coveralls commented Sep 19, 2016