[FEATURE] Add seqan3::sam_flag enum and adapt alignment io. #1390

smehringer · 2019-11-29T10:36:04Z

h-2 · 2019-11-29T10:52:13Z

include/seqan3/io/alignment_file/misc.hpp

+ */
+enum class sam_flag : uint16_t
+{
+   paired                  = 0x1,   //!< The aligned read is paired (paired-end sequencing).


Maybe add none with value zero so it can be initialised and reset without passing {0} which is not very expressive for an enum.

codecov · 2019-12-03T09:47:19Z

Codecov Report

Merging #1390 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
+ Coverage   97.53%   97.53%   +<.01%     
==========================================
  Files         228      229       +1     
  Lines        8731     8735       +4     
==========================================
+ Hits         8516     8520       +4     
  Misses        215      215

Impacted Files	Coverage Δ
include/seqan3/io/alignment_file/input.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/alignment_file/format_bam.hpp	`95.72% <ø> (ø)`	⬆️
.../seqan3/io/alignment_file/input_format_concept.hpp	`100% <ø> (ø)`	⬆️
...seqan3/io/alignment_file/output_format_concept.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/alignment_file/format_sam.hpp	`98.39% <100%> (+0.01%)`	⬆️
include/seqan3/io/alignment_file/output.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/io/alignment_file/misc.hpp	`100% <100%> (ø)`
include/seqan3/argument_parser/validators.hpp	`88.69% <0%> (ø)`	⬆️

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 6eba644...399896e. Read the comment docs.

joshuak94

Do you think it would be necessary to implement any sort of checks for legal combinations, or is that better left to the user to not do anything silly. (i.e. 0x40 and 0x80 don't make sense to both be set)

smehringer · 2019-12-03T11:53:42Z

Since silly combinations are not formalized I would not do any checks by default. We might consider implementing a free function like validate_sam_flags that checks for common mistakes but this will not be part of this PR

joshuak94

Looks good!

rrahn

looks good. Only minor stuff 💅

include/seqan3/io/alignment_file/format_bam.hpp

rrahn · 2019-12-03T16:10:25Z

include/seqan3/io/alignment_file/format_sam.hpp

@@ -633,7 +651,7 @@ inline void format_sam::write_alignment_record(stream_type & stream,
                                               std::optional<int32_t> ref_offset,
                                               align_type && align,
                                               std::vector<cigar> const & cigar_vector,
-                                               uint16_t flag,
+                                               sam_flag flag,


is the flag being modified within the method? If not please mark it const.

rrahn · 2019-12-03T16:10:39Z

include/seqan3/io/alignment_file/format_sam.hpp

@@ -633,7 +651,7 @@ inline void format_sam::write_alignment_record(stream_type & stream,
                                               std::optional<int32_t> ref_offset,
                                               align_type && align,
                                               std::vector<cigar> const & cigar_vector,
-                                               uint16_t flag,
+                                               sam_flag flag,
                                               uint8_t mapq,


same here if applicable.

I also changed this in format_bam

rrahn · 2019-12-03T16:16:21Z

test/unit/io/alignment_file/alignment_file_format_test_template.hpp

@@ -15,7 +15,7 @@
 #include <seqan3/io/alignment_file/input.hpp>
 #include <seqan3/io/alignment_file/output.hpp>
 #include <seqan3/range/views/take.hpp>
-#include <seqan3/test/pretty_printing.hpp>
+// #include <seqan3/test/pretty_printing.hpp>


Some left over?

yes, thanks

rrahn · 2019-12-03T16:17:05Z

test/snippet/io/alignment_file/sam_flags.cpp

+#include <iostream>
+#include <sstream>
+
+#include <seqan3/core/type_list/type_list.hpp>


why do you need type_list?

Good catch 🙈

rrahn · 2019-12-03T18:42:21Z

snippet tests are failing as well.

smehringer · 2019-12-05T08:39:02Z

Note that I additionally added a debug_stream test for sam_flags since I added an overload.
I also updated the changelog. I need to clean the commit history before merging

rrahn

some minor stuff left 💅

rrahn · 2019-12-05T19:44:49Z

CHANGELOG.md

@@ -58,6 +58,10 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.
 * **The `type_list` header has moved:**
  If you included `<seqan3/core/type_list.hpp>` you need to change the path to `<seqan3/core/type_list/type_list.hpp>`.

+#### I/O
+
+* The `field::FLAG` of SAM/BAM input and output is not an enum instead of a simple integer (see seqan3::sam_flag).


Suggested change

* The `field::FLAG` of SAM/BAM input and output is not an enum instead of a simple integer (see seqan3::sam_flag).

* The `field::FLAG` of SAM/BAM input and output is now an enum instead of a simple integer (see seqan3::sam_flag).

This is an API change. Can you please document how this will affect people and how to fix possible compiler errors.

I don't really know what to say here...?
You cannot use integers anymore when writing but you need to static_cast your integers into the respective enum value?
And when you read, well you need to change all your types from integer to sam_flags wherever you use them..

rrahn · 2019-12-05T19:46:43Z

include/seqan3/io/alignment_file/format_bam.hpp

@@ -394,7 +395,7 @@ inline void format_bam::read_alignment_record(stream_type & stream,
        ref_id = core.refID;                                                   // field::REF_ID
    }

-    flag = core.flag;                                                          // field::FLAG
+    flag = sam_flag{static_cast<uint16_t>(core.flag)};                         // field::FLAG


why is core.flag not already an uin16_t?

Because I copied the core structure from seqan2 aand I did not know that using bit fields is obsolete if you use the correct integers...

rrahn · 2019-12-05T19:49:07Z

test/snippet/io/alignment_file/sam_flags.cpp

+    {
+        // Check if a certain flag value (bit) is set:
+        if (static_cast<bool>(seqan3::get<seqan3::field::FLAG>(rec) & seqan3::sam_flag::unmapped))
+        {


you could remove the braces

rrahn · 2019-12-05T19:51:19Z

test/unit/core/debug_stream_test.cpp

+
+TEST(debug_stream_test, sam_flags)
+{
+    std::ostringstream o;


Suggested change

std::ostringstream o;

std::ostringstream o{};

rrahn · 2019-12-06T19:55:39Z

include/seqan3/io/alignment_file/format_bam.hpp

@@ -395,7 +395,7 @@ inline void format_bam::read_alignment_record(stream_type & stream,
        ref_id = core.refID;                                                   // field::REF_ID
    }

-    flag = sam_flag{static_cast<uint16_t>(core.flag)};                         // field::FLAG
+    flag = core.flag;                                                          // field::FLAG


rrahn

lgtm

h-2 reviewed Nov 29, 2019

View reviewed changes

smehringer force-pushed the io_misc branch 2 times, most recently from 5793ede to bc87dc4 Compare December 3, 2019 09:47

smehringer requested a review from joshuak94 December 3, 2019 09:50

joshuak94 reviewed Dec 3, 2019

View reviewed changes

joshuak94 self-requested a review December 3, 2019 11:54

joshuak94 approved these changes Dec 3, 2019

View reviewed changes

smehringer force-pushed the io_misc branch from bc87dc4 to 1b7853f Compare December 3, 2019 11:58

smehringer requested a review from rrahn December 3, 2019 12:05

rrahn requested changes Dec 3, 2019

View reviewed changes

smehringer requested a review from rrahn December 5, 2019 14:41

rrahn requested changes Dec 5, 2019

View reviewed changes

smehringer requested a review from rrahn December 6, 2019 13:57

rrahn reviewed Dec 6, 2019

View reviewed changes

rrahn approved these changes Dec 6, 2019

View reviewed changes

smehringer force-pushed the io_misc branch from b3a5d52 to 112aaf1 Compare December 9, 2019 15:23

smehringer added 2 commits December 9, 2019 16:25

[FEATURE] Add seqan3::sam_flag enum and adapt alignment io.

245fe99

[MISC] make write parameters const.

399896e

smehringer force-pushed the io_misc branch from 112aaf1 to 399896e Compare December 9, 2019 15:25

rrahn merged commit 62d03a2 into seqan:master Dec 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add seqan3::sam_flag enum and adapt alignment io. #1390

[FEATURE] Add seqan3::sam_flag enum and adapt alignment io. #1390

smehringer commented Nov 29, 2019

h-2 Nov 29, 2019

codecov bot commented Dec 3, 2019 •

edited

Loading

joshuak94 left a comment

smehringer commented Dec 3, 2019

joshuak94 left a comment

rrahn left a comment

rrahn Dec 3, 2019

rrahn Dec 3, 2019

smehringer Dec 5, 2019

rrahn Dec 3, 2019

smehringer Dec 5, 2019 •

edited

Loading

rrahn Dec 3, 2019

smehringer Dec 5, 2019

rrahn commented Dec 3, 2019

smehringer commented Dec 5, 2019

rrahn left a comment

rrahn Dec 5, 2019

smehringer Dec 6, 2019

smehringer Dec 9, 2019

rrahn Dec 5, 2019

smehringer Dec 6, 2019

rrahn Dec 5, 2019

rrahn Dec 5, 2019

rrahn Dec 6, 2019

rrahn left a comment

	* The `field::FLAG` of SAM/BAM input and output is not an enum instead of a simple integer (see seqan3::sam_flag).
	* The `field::FLAG` of SAM/BAM input and output is now an enum instead of a simple integer (see seqan3::sam_flag).

[FEATURE] Add seqan3::sam_flag enum and adapt alignment io. #1390

[FEATURE] Add seqan3::sam_flag enum and adapt alignment io. #1390

Conversation

smehringer commented Nov 29, 2019

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2019 • edited Loading

Codecov Report

joshuak94 left a comment

Choose a reason for hiding this comment

smehringer commented Dec 3, 2019

joshuak94 left a comment

Choose a reason for hiding this comment

rrahn 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

smehringer Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn commented Dec 3, 2019

smehringer commented Dec 5, 2019

rrahn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2019 •

edited

Loading

smehringer Dec 5, 2019 •

edited

Loading