[FIX] Parsing BAM headers with 64 reference sequences #2423

eseiler · 2021-03-03T10:23:10Z

Resolves #2422

Todo:

BAM works, SAM does not
Test
Changelog
Check for performance regression

vercel · 2021-03-03T10:23:14Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/86UZWL3SkiF2gqcubyBuS4Zh1gdz
✅ Preview: https://seqan3-git-fork-eseiler-fix-bam2-seqan.vercel.app

eseiler · 2021-03-03T10:41:24Z

include/seqan3/io/sam_file/format_sam_base.hpp

    };

-    while (is_char<'@'>(*it))
+    while (it != end && is_char<'@'>(*it))


it != end is used in BAM, the stream_view is a view_take_exactly_or_throw and we need to stop at the end (the size is known).

is_char<'@'>(*it) is used in SAM, the stream_view is a view over the file and we stop when we read all lines that start with @.

codecov · 2021-03-03T10:53:50Z

Codecov Report

Merging #2423 (674a178) into master (3cd33a0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2423   +/-   ##
=======================================
  Coverage   98.23%   98.24%           
=======================================
  Files         267      267           
  Lines       10900    10917   +17     
=======================================
+ Hits        10708    10725   +17     
  Misses        192      192

Impacted Files	Coverage Δ
include/seqan3/io/sam_file/format_sam_base.hpp	`98.12% <100.00%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803ee1d...7d0230f. Read the comment docs.

include/seqan3/io/sam_file/format_sam_base.hpp

eseiler · 2021-03-03T15:55:29Z

test/unit/io/sam_file/format_bam_test.cpp

@@ -179,6 +179,47 @@ struct sam_file_read<seqan3::format_bam> : public sam_file_data
        '\x66', '\x66', '\x66', '\x46', '\x40', '\x7A', '\x7A', '\x5A', '\x73', '\x74', '\x72', '\x00'
    };

+    std::string many_refs{[] ()


The BAM would be multiple hundred lines long, that's why it is generated.

The comments + BAM specification make it feasible to understand.

We could have also generated a bam file and added it as file :D

eseiler · 2021-03-03T16:01:05Z

CHANGELOG.md

@@ -80,6 +80,7 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.

 * Requesting the alignment without also requesting the sequence for BAM files containing empty CIGAR strings does now
  not result in erroneous parsing ([\#2418](https://github.com/seqan/seqan3/pull/2418)).
+* BAM files with 64 references are now parsed correctly ([\#2423](https://github.com/seqan/seqan3/pull/2423)).


Technically it's every number of references whose last byte is \x40, e.g. it's also affecting 320, but I didn't come up with a concise way to describe this.

Well, it works properly for 128. The first/last(?) byte has to look like 01000000 or 11000000, i.e. the first least significant bit to be set is the seventh...

you are right: (mod 256) == 64

joergi-w

I checked this PR regarding the syntax and what I know about I/O, but please get another review from someone who knows the sam/bam specific semantics better than me...

joergi-w · 2021-03-03T16:10:26Z

include/seqan3/io/sam_file/format_sam_base.hpp

+    auto take_until_colon_and_consume = [&it] ()
+    {
+        while (!is_char<':'>(*it))
+            ++it;
+        ++it;
+    };


views::take_until(is_char<':'>) doesn't work here?

No, see the issue.
We would make a copy of the stream_view and hence not update the size of the orignal stream_view

marehr

thank you :)

marehr · 2021-03-04T00:19:14Z

CHANGELOG.md

@@ -80,6 +80,7 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.

 * Requesting the alignment without also requesting the sequence for BAM files containing empty CIGAR strings does now
  not result in erroneous parsing ([\#2418](https://github.com/seqan/seqan3/pull/2418)).
+* BAM files with 64 references are now parsed correctly ([\#2423](https://github.com/seqan/seqan3/pull/2423)).


include/seqan3/io/sam_file/format_sam_base.hpp

marehr · 2021-03-04T00:27:40Z

include/seqan3/io/sam_file/format_sam_base.hpp

@@ -459,16 +459,34 @@ inline void format_sam_base::read_header(stream_view_type && stream_view,
                                         ref_seqs_type & /*ref_id_to_pos_map*/)
 {
    auto it = std::ranges::begin(stream_view);
+    auto end = std::ranges::end(stream_view);
+    std::string string_buffer{};


Please use vector here, because std::string can be quite slower because of the small string optimization

marehr · 2021-03-04T00:29:19Z

test/unit/io/sam_file/format_bam_test.cpp

@@ -179,6 +179,47 @@ struct sam_file_read<seqan3::format_bam> : public sam_file_data
        '\x66', '\x66', '\x66', '\x46', '\x40', '\x7A', '\x7A', '\x5A', '\x73', '\x74', '\x72', '\x00'
    };

+    std::string many_refs{[] ()


We could have also generated a bam file and added it as file :D

marehr

very nice!

vercel bot deployed to Preview March 3, 2021 10:23 View deployment

eseiler mentioned this pull request Mar 3, 2021

SeqAn parses BAM headers past the value given in l_text #2422

Closed

eseiler marked this pull request as draft March 3, 2021 10:29

eseiler force-pushed the fix/bam2 branch from 939a603 to 6bccd2a Compare March 3, 2021 10:38

vercel bot deployed to Preview March 3, 2021 10:39 View deployment

eseiler commented Mar 3, 2021

View reviewed changes

include/seqan3/io/sam_file/format_sam_base.hpp Outdated Show resolved Hide resolved

vercel bot deployed to Preview March 3, 2021 11:28 View deployment

eseiler commented Mar 3, 2021

View reviewed changes

include/seqan3/io/sam_file/format_sam_base.hpp Outdated Show resolved Hide resolved

eseiler marked this pull request as ready for review March 3, 2021 11:38

eseiler requested review from a team and joergi-w and removed request for a team March 3, 2021 11:39

vercel bot deployed to Preview March 3, 2021 11:39 View deployment

eseiler force-pushed the fix/bam2 branch from d7d9ff1 to 2cf2eca Compare March 3, 2021 15:53

vercel bot deployed to Preview March 3, 2021 15:53 View deployment

eseiler commented Mar 3, 2021

View reviewed changes

joergi-w approved these changes Mar 3, 2021

View reviewed changes

eseiler requested review from a team and marehr and removed request for a team March 3, 2021 17:30

marehr reviewed Mar 4, 2021

View reviewed changes

eseiler force-pushed the fix/bam2 branch from 69917c5 to e5296e7 Compare March 4, 2021 08:15

eseiler requested a review from marehr March 4, 2021 08:15

vercel bot deployed to Preview March 4, 2021 08:15 View deployment

[FIX] Parsing BAM headers with 64 reference sequences

7d0230f

eseiler force-pushed the fix/bam2 branch from e5296e7 to 7d0230f Compare March 4, 2021 08:16

vercel bot deployed to Preview March 4, 2021 08:24 View deployment

marehr approved these changes Mar 4, 2021

View reviewed changes

marehr merged commit a0e4787 into seqan:master Mar 4, 2021

eseiler deleted the fix/bam2 branch May 14, 2021 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Parsing BAM headers with 64 reference sequences #2423

[FIX] Parsing BAM headers with 64 reference sequences #2423

eseiler commented Mar 3, 2021 •

edited

vercel bot commented Mar 3, 2021 •

edited

eseiler Mar 3, 2021 •

edited

codecov bot commented Mar 3, 2021 •

edited

eseiler Mar 3, 2021 •

edited

marehr Mar 4, 2021

eseiler Mar 3, 2021 •

edited

marehr Mar 4, 2021

eseiler Mar 4, 2021 •

edited

marehr Mar 4, 2021

joergi-w left a comment

joergi-w Mar 3, 2021

eseiler Mar 3, 2021

marehr left a comment

marehr Mar 4, 2021

marehr Mar 4, 2021

marehr Mar 4, 2021

marehr left a comment

[FIX] Parsing BAM headers with 64 reference sequences #2423

[FIX] Parsing BAM headers with 64 reference sequences #2423

Conversation

eseiler commented Mar 3, 2021 • edited

vercel bot commented Mar 3, 2021 • edited

eseiler Mar 3, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2021 • edited

Codecov Report

eseiler Mar 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler Mar 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler Mar 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr 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

marehr left a comment

Choose a reason for hiding this comment

eseiler commented Mar 3, 2021 •

edited

vercel bot commented Mar 3, 2021 •

edited

eseiler Mar 3, 2021 •

edited

codecov bot commented Mar 3, 2021 •

edited

eseiler Mar 3, 2021 •

edited

eseiler Mar 3, 2021 •

edited

eseiler Mar 4, 2021 •

edited