[FIX] 3 minor changes in alignment IO #1110

smehringer · 2019-06-20T09:50:31Z

BAM needs to write *\0 for empty read ids (not \0 like right now, which did not impede our files since they are equivalent but for samtools those are not equivalent)
SAM should not have requirements on types it does not use. (This way you could also pass std::ignore to the file)
The read id alphabet should not be modifiable by the traits object since there is no use case other than char (container is modifiable) and char is required by BAM. (Is this an API break?)

eseiler

One question.
I don't think that 3 is an API break, but not too sure.

include/seqan3/io/alignment_file/format_sam.hpp

codecov · 2019-06-20T10:41:02Z

Codecov Report

Merging #1110 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   96.42%   96.45%   +0.02%     
==========================================
  Files         196      196              
  Lines        7757     7749       -8     
==========================================
- Hits         7480     7474       -6     
+ Misses        277      275       -2

Impacted Files	Coverage Δ
include/seqan3/io/alignment_file/header.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/alignment_file/format_bam.hpp	`95.4% <100%> (+0.32%)`	⬆️
include/seqan3/io/alignment_file/format_sam.hpp	`98.98% <100%> (ø)`	⬆️
include/seqan3/io/alignment_file/input.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/io/alignment_file/output.hpp	`100% <100%> (ø)`	⬆️
...de/seqan3/argument_parser/detail/version_check.hpp	`82.73% <0%> (-0.13%)`	⬇️
include/seqan3/search/fm_index/fm_index.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/io/alignment_file/detail.hpp	`94.28% <0%> (+0.57%)`	⬆️

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 f4e8fd4...4b63d48. Read the comment docs.

include/seqan3/io/alignment_file/format_bam.hpp

h-2 · 2019-06-20T13:54:39Z

The read id alphabet should not be modifiable by the traits object since there is no use case other than char (container is modifiable) and char is required by BAM. (Is this an API break?)

Why would anything ConvertibleTo<char> not be ok? Currently char8_t would work for example, it will just be converted. Strictly speaking you should check ConvertibleTo Stream's char_type (this has implicitly been the case right now so changing to this behaviour is not breaking) and maybe restrict that further if that actually can't be wchar_t.

SAM should not have requirements on types it does not use. (This way you could also pass std::ignore to the file)

In general all the static assert could/should be moved to the file, or not? They are the same for all formats anyway, because all formats are instantiated with all parameters. The dynamic asserts via exceptions are format specific and should be with the format...

smehringer · 2019-06-20T13:59:31Z

In general all the static assert could/should be moved to the file, or not? They are the same for all formats anyway, because all formats are instantiated with all parameters. The dynamic asserts via exceptions are format specific and should be with the format...

Yes you are right. Shall I do this in this PR or another?

h-2 · 2019-06-20T14:05:08Z

Yes you are right. Shall I do this in this PR or another?

However you like!

include/seqan3/alphabet/detail/convert.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/format_sam.hpp

include/seqan3/io/alignment_file/input.hpp

include/seqan3/io/alignment_file/format_bam.hpp

h-2 · 2019-07-02T12:00:06Z

include/seqan3/alphabet/detail/convert.hpp

@@ -39,7 +39,7 @@ constexpr std::array<out_t, alphabet_size<in_t>> convert_through_char_representa
    [] () constexpr
    {
        std::array<out_t, alphabet_size<in_t>> ret{};
-        for (typename in_t::rank_type i = 0; i < alphabet_size<in_t>; ++i)
+        for (detail::min_viable_uint_t<alphabet_size<in_t>> i = 0; i < alphabet_size<in_t>; ++i)


Suggested change

for (detail::min_viable_uint_t<alphabet_size<in_t>> i = 0; i < alphabet_size<in_t>; ++i)

for (decltype(alphabet_size<in_t>) i = 0; i < alphabet_size<in_t>; ++i)

I thought these are eqivalent

If I make that change, the bam test compiles forever 😱

for (auto i = decltype(alphabet_size<in_t>){0}; i < alphabet_size<in_t>; ++i) funktioniert wieder ^^

If I make that change, the bam test compiles forever scream

It should actually be faster. Keep in mind that detail::min_viable_uint_t< instantiates a type, whereas decltype does not.

for (auto i = decltype(alphabet_size<in_t>){0}; i < alphabet_size<in_t>; ++i)

whut?

🤷‍♀️ I dunno 🤷‍♀️

This is still weird and it is not guaranteed that the min_viable_uint_type is the correct type. Just because we decide to do that doesn't mean other alphabets don't use size_t so this will result in conversion. I don't understand why decltype should cause problems here. If it does, please document it in the code.

hm if decltype is more correct, i can use the second version?

for (auto i = decltype(alphabet_size<in_t>){0}; i < alphabet_size<in_t>; ++i)

alright. I changed it and made a comment that it does not work the other way

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/header.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/input.hpp

…t of the container.

…tibility.

…nyway.

…ge this and it is required in BAM.

smehringer · 2019-07-04T06:14:34Z

Jenkins says 100% passed but still shows a failure 🤷‍♀️
@h-2 can we merge this now?

smehringer requested a review from eseiler June 20, 2019 09:50

eseiler requested changes Jun 20, 2019

View reviewed changes

include/seqan3/io/alignment_file/format_sam.hpp Show resolved Hide resolved

eseiler approved these changes Jun 20, 2019

View reviewed changes

h-2 reviewed Jun 20, 2019

View reviewed changes

include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the alignment_file_in branch from bc32256 to 5d3eb9a Compare June 20, 2019 15:16

smehringer requested a review from eseiler June 20, 2019 15:16

eseiler requested changes Jun 21, 2019

View reviewed changes

include/seqan3/alphabet/detail/convert.hpp Outdated Show resolved Hide resolved

include/seqan3/alphabet/detail/convert.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the alignment_file_in branch from 5d3eb9a to c68a316 Compare June 24, 2019 14:56

smehringer requested a review from eseiler June 24, 2019 14:58

eseiler approved these changes Jun 25, 2019

View reviewed changes

smehringer requested a review from h-2 June 25, 2019 11:59

h-2 requested changes Jun 25, 2019

View reviewed changes

smehringer requested a review from h-2 June 27, 2019 06:19

smehringer force-pushed the alignment_file_in branch 2 times, most recently from d10fd89 to 0bd1703 Compare June 27, 2019 07:24

h-2 reviewed Jun 27, 2019

View reviewed changes

include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the alignment_file_in branch from 0bd1703 to 7856b91 Compare June 28, 2019 12:46

smehringer requested a review from h-2 June 28, 2019 12:46

smehringer force-pushed the alignment_file_in branch 2 times, most recently from 35c5d44 to ba7b872 Compare July 1, 2019 14:19

h-2 requested changes Jul 2, 2019

View reviewed changes

smehringer force-pushed the alignment_file_in branch from ba7b872 to 38c6901 Compare July 2, 2019 13:04

smehringer requested a review from h-2 July 2, 2019 13:04

smehringer force-pushed the alignment_file_in branch from 38c6901 to ea9b447 Compare July 2, 2019 14:02

smehringer added 2 commits July 3, 2019 15:27

[FIX] rank_type overflows for in_t == char.

feff877

[FIX] Allow Contigous ranges to be queried in the ref_dict independen…

18017bb

…t of the container.

smehringer added 4 commits July 3, 2019 15:27

[FIX] Empty id needs to be written as '*\0' in BAM for backward compa…

d9bb7ab

…tibility.

[FIX] SAM should not have requirements on the types it does not use a…

84ffea7

…nyway.

[MISC] Restrict the id alphabet to char. There is no use case to chan…

2d792c8

…ge this and it is required in BAM.

[FIX] Truncate read ids that are longer than 255 chars when writing BAM.

4b63d48

smehringer force-pushed the alignment_file_in branch from ea9b447 to 4b63d48 Compare July 3, 2019 13:27

h-2 approved these changes Jul 4, 2019

View reviewed changes

h-2 merged commit f4be633 into seqan:master Jul 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] 3 minor changes in alignment IO #1110

[FIX] 3 minor changes in alignment IO #1110

smehringer commented Jun 20, 2019

eseiler left a comment •

edited

Loading

codecov bot commented Jun 20, 2019 •

edited

Loading

h-2 commented Jun 20, 2019

smehringer commented Jun 20, 2019

h-2 commented Jun 20, 2019

h-2 Jul 2, 2019

smehringer Jul 2, 2019

smehringer Jul 2, 2019

smehringer Jul 2, 2019

h-2 Jul 2, 2019

smehringer Jul 2, 2019

h-2 Jul 3, 2019

smehringer Jul 3, 2019

h-2 Jul 3, 2019

smehringer Jul 3, 2019

smehringer commented Jul 4, 2019 •

edited

Loading

	for (detail::min_viable_uint_t<alphabet_size<in_t>> i = 0; i < alphabet_size<in_t>; ++i)
	for (decltype(alphabet_size<in_t>) i = 0; i < alphabet_size<in_t>; ++i)

[FIX] 3 minor changes in alignment IO #1110

[FIX] 3 minor changes in alignment IO #1110

Conversation

smehringer commented Jun 20, 2019

eseiler left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2019 • edited Loading

Codecov Report

h-2 commented Jun 20, 2019

smehringer commented Jun 20, 2019

h-2 commented Jun 20, 2019

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

Choose a reason for hiding this comment

smehringer commented Jul 4, 2019 • edited Loading

eseiler left a comment •

edited

Loading

codecov bot commented Jun 20, 2019 •

edited

Loading

smehringer commented Jul 4, 2019 •

edited

Loading