Add support for Sam Header Readgroup Barcode field #1210

lbergelson · 2018-10-26T19:56:43Z

adding support in SAMReadGroupRecord for the BC attribute
this was added to the 1.6 spec

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)

codecov-io · 2018-10-26T20:24:56Z

Codecov Report

Merging #1210 into master will increase coverage by 0.014%.
The diff coverage is 83.333%.

@@               Coverage Diff               @@
##              master     #1210       +/-   ##
===============================================
+ Coverage     69.358%   69.372%   +0.014%     
- Complexity      8302      8308        +6     
===============================================
  Files            555       555               
  Lines          33118     33130       +12     
  Branches        5572      5575        +3     
===============================================
+ Hits           22970     22983       +13     
+ Misses          7886      7885        -1     
  Partials        2262      2262

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/htsjdk/samtools/SAMReadGroupRecord.java	`94.286% <83.333%> (-2.266%)`	`48 <5> (+5)`
...samtools/util/AsyncBlockCompressedInputStream.java	`76% <0%> (+4%)`	`13% <0%> (+1%)`	⬆️

magicDGS

It will be nice to be able to configure the barcode separator somehow. Also, it might be possible to have a way to override the default statically, and like that an user can change it on startup to have a different implementation.

magicDGS · 2018-10-28T11:32:08Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

+    /**
+     * @return the List of barcodes associated with this read group or null
+     */
+    public List<String> getBarcodes() {


I would add a method using a different barcode separator too (in case it is needed by other implementations), and the one without params should use the default BARCODE_SEPARATOR

@magicDGS
The spec recommends using - as the separator:

Barcode sequence identifying the sample or library. This value is the expected barcode bases as read by the sequencing machine in the absence of errors. If there are several barcodes for the sample/library (e.g., one on each end of the template), the recommended implementation concatenates all the barcodes separating them with hyphens (‘-’).

My though was that we should support the recommended mode and not go out of the way to support other modes. You can always get around it by using getAttribute directly and parsing them yourself.

Are there implementations out there already? Is there a practical need for what you suggest or just a theoretical one?

I found some FASTQ files with + as barcode separator, that when are converted with custom scripts to SAM/BAM the @RG tag (per-read) keeps that symbol; thus, some users of ReadTools have asked me to support other separators, which I did with custom code, but as a recommendation in the specs it looks like having a method supporting other implementations will be nice for the library.

Anyway, I am planning to come back in the real near future to htsjk3, where we can take the decisions about supporting the "recommended" specs alone or all of them.

I think that adding explicit support for non-standard barcode separators adds unnecessary complication. If you need to parse special ones you can always use getAttribute and setAttribute

magicDGS · 2018-10-28T11:32:35Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

+    /**
+     * The recommended separator for the {@link #BARCODE_TAG} when there are multiple bar codes associated with this read group.
+     */
+    public static final String BARCODE_SEPARATOR = "-";


Maybe a better name will be DEFAULT_BARCODE_SEPARATOR.

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

pshapiro4broad · 2018-10-30T15:41:52Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

+            return null;
+        } else if( barcodeString.isEmpty()){
+            return Collections.emptyList();
+        } else {


else unnecessary after return

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

src/test/java/htsjdk/samtools/SAMReadGroupRecordTest.java

cmnbroad

One suggestion and one comment about merging headers with RGs with barcodes. It looks like the header merger code merges RGs that have the same ID and identical attributes, but keeps them separate (by remapping the colliding ID) if they have the same ID but different attributes. So I think RGs that are identical except for the barcode separator would be kept separate, even if the barcode were identical, which is probably as good as we can do.

cmnbroad · 2018-11-06T20:35:43Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

@@ -51,6 +47,13 @@
    public static final String PLATFORM_MODEL_TAG = "PM";
    public static final String PLATFORM_UNIT_TAG = "PU";
    public static final String READ_GROUP_SAMPLE_TAG = "SM";
+    public static final String BARCODE_TAG = "BC";


I think this should be added to STANDARD_TAGS (?)

yes, good idea

src/test/java/htsjdk/samtools/SAMReadGroupRecordTest.java

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

src/test/java/htsjdk/samtools/SAMReadGroupRecordTest.java

lbergelson · 2018-11-19T19:17:58Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

+    /**
+     * @return the List of barcodes associated with this read group or null
+     */
+    public List<String> getBarcodes() {


I think that adding explicit support for non-standard barcode separators adds unnecessary complication. If you need to parse special ones you can always use getAttribute and setAttribute

lbergelson · 2018-11-19T19:21:05Z

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java

@@ -51,6 +47,13 @@
    public static final String PLATFORM_MODEL_TAG = "PM";
    public static final String PLATFORM_UNIT_TAG = "PU";
    public static final String READ_GROUP_SAMPLE_TAG = "SM";
+    public static final String BARCODE_TAG = "BC";


yes, good idea

yfarjoun

a few nits. but fine otherwise

yfarjoun · 2018-12-03T19:06:42Z

why the forced-push?

* adding support in SAMReadGroupRecord for the BC attribute * this was added to the 1.6 spec

lbergelson · 2019-01-24T20:38:04Z

@yfarjoun Can you take a quick look at the latest changes here? I realized the constants of yours I just merged weren't public so I made them as and added a javadoc.

src/main/java/htsjdk/samtools/util/SamConstants.java

Co-Authored-By: lbergelson <[email protected]>

lbergelson mentioned this pull request Oct 26, 2018

add SAM 1.6 to supported versions #1186

Open

magicDGS suggested changes Oct 28, 2018

View reviewed changes

pshapiro4broad reviewed Oct 30, 2018

View reviewed changes

cmnbroad reviewed Nov 7, 2018

View reviewed changes

yfarjoun reviewed Nov 19, 2018

View reviewed changes

src/test/java/htsjdk/samtools/SAMReadGroupRecordTest.java Outdated Show resolved Hide resolved

yfarjoun reviewed Nov 19, 2018

View reviewed changes

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java Outdated Show resolved Hide resolved

yfarjoun reviewed Nov 19, 2018

View reviewed changes

src/main/java/htsjdk/samtools/SAMReadGroupRecord.java Outdated Show resolved Hide resolved

lbergelson commented Nov 19, 2018

View reviewed changes

yfarjoun approved these changes Nov 19, 2018

View reviewed changes

lbergelson force-pushed the lb_RG_BC branch from 6b8af03 to 013be18 Compare November 19, 2018 19:33

yfarjoun added the Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond label Dec 3, 2018

lbergelson added 2 commits January 24, 2019 15:29

Add support for Sam Header Readgroup Barcode field

4bd1b40

* adding support in SAMReadGroupRecord for the BC attribute * this was added to the 1.6 spec

use new tag constants

78d5001

lbergelson force-pushed the lb_RG_BC branch from 013be18 to 78d5001 Compare January 24, 2019 20:37

yfarjoun reviewed Jan 24, 2019

View reviewed changes

src/main/java/htsjdk/samtools/util/SamConstants.java Outdated Show resolved Hide resolved

Update src/main/java/htsjdk/samtools/util/SamConstants.java

85947c6

Co-Authored-By: lbergelson <[email protected]>

yfarjoun approved these changes Jan 24, 2019

View reviewed changes

Apply suggestions from code review

65eb398

Co-Authored-By: lbergelson <[email protected]>

lbergelson merged commit 5ef9223 into master Jan 29, 2019

lbergelson deleted the lb_RG_BC branch January 29, 2019 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Sam Header Readgroup Barcode field #1210

Add support for Sam Header Readgroup Barcode field #1210

lbergelson commented Oct 26, 2018

codecov-io commented Oct 26, 2018 •

edited

Loading

magicDGS left a comment

magicDGS Oct 28, 2018

lbergelson Nov 7, 2018

magicDGS Nov 8, 2018

lbergelson Nov 19, 2018

magicDGS Oct 28, 2018

pshapiro4broad Oct 30, 2018

cmnbroad left a comment

cmnbroad Nov 6, 2018

lbergelson Nov 19, 2018

lbergelson Nov 19, 2018

lbergelson Nov 19, 2018

yfarjoun left a comment

yfarjoun commented Dec 3, 2018

lbergelson commented Jan 24, 2019 •

edited

Loading

Add support for Sam Header Readgroup Barcode field #1210

Add support for Sam Header Readgroup Barcode field #1210

Conversation

lbergelson commented Oct 26, 2018

Checklist

codecov-io commented Oct 26, 2018 • edited Loading

Codecov Report

magicDGS left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a 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

Choose a reason for hiding this comment

yfarjoun left a comment

Choose a reason for hiding this comment

yfarjoun commented Dec 3, 2018

lbergelson commented Jan 24, 2019 • edited Loading

codecov-io commented Oct 26, 2018 •

edited

Loading

lbergelson commented Jan 24, 2019 •

edited

Loading