[MISC] Remove field::alignment from sam_file_[in/out]out. #3058

smehringer · 2022-10-04T09:27:53Z

Also Fixes #2907

Deprecation warning:

The most readable solution is to add a static assert. Otherwise I would have to deprecate the field::alignment. But then we couldn't use it once we implement e.g. Blast or MSA IO.
So I just wanted to deprecate using it with the SAM file.

In file included from /home/svenja/AGABI/seqan3/test/snippet/io/sam_file/sam_file_input_reading_structured_bindings.cpp:4:
/home/svenja/AGABI/seqan3/include/seqan3/io/sam_file/input.hpp: In instantiation of ‘class seqan3::sam_file_input<seqan3::sam_file_input_default_traits<>, seqan3::fields<seqan3::field::alignment, seqan3::field::mapq>, seqan3::type_list<seqan3::format_sam> >’:
/home/svenja/AGABI/seqan3/test/snippet/io/sam_file/sam_file_input_reading_structured_bindings.cpp:19:95:   required from here
/home/svenja/AGABI/seqan3/include/seqan3/io/sam_file/input.hpp:260:19: error: static assertion failed: The field::alignment is deprecated and only field::cigar is supported. Please see seqan3::alignment_from_cigar on how to get an alignment from the cigar information.
  260 |     static_assert(!selected_field_ids::contains(field::alignment),

Todo:

Check documentation

Follow Up:

Do we need to pass the reference sequences now that the field::alignment is gone
Remove field::offset

vercel · 2022-10-04T09:27:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
seqan3	✅ Ready (Inspect)	Visit Preview	Nov 1, 2022 at 11:09AM (UTC)

codecov · 2022-10-05T11:05:12Z

Codecov Report

Base: 98.24% // Head: 98.22% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (049eb2f) compared to base (334b790).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   98.24%   98.22%   -0.02%     
==========================================
  Files         275      275              
  Lines       12346    12197     -149     
==========================================
- Hits        12129    11981     -148     
+ Misses        217      216       -1

Impacted Files	Coverage Δ
include/seqan3/alphabet/cigar/cigar.hpp	`90.90% <ø> (ø)`
include/seqan3/io/sam_file/detail/cigar.hpp	`100.00% <ø> (+1.96%)`	⬆️
...lude/seqan3/io/sam_file/detail/format_sam_base.hpp	`97.85% <ø> (-0.12%)`	⬇️
include/seqan3/io/sam_file/header.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/input.hpp	`100.00% <ø> (ø)`
...nclude/seqan3/io/sam_file/input_format_concept.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/output.hpp	`100.00% <ø> (ø)`
...clude/seqan3/io/sam_file/output_format_concept.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/record.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/sam_flag.hpp	`100.00% <ø> (ø)`
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

smehringer · 2022-10-25T11:27:48Z

I don't think the coverage fail is correct (maybe just telling me that I deleted a lot of lines)

doc/fragments/io_sam_file_input.md

doc/tutorial/10_sam_file/index.md

include/seqan3/io/sam_file/input.hpp

eseiler · 2022-10-31T14:48:30Z

test/unit/io/sam_file/format_bam_test.cpp

-    auto alignment = std::tie(gapped_ref, gapped_read);
+    // create a cigar with more than 65535 cigar elements
+    std::vector<seqan3::cigar> too_long_cigar{};
+    for (size_t i = 0; i < 69'999; ++i)


Suggested change

for (size_t i = 0; i < 69'999; ++i)

for (size_t i = 0; i <= 65'535; ++i)

Should already suffice?

yes but is it necessary? I would need to change the BAM file binary tests

Then it can stay as is, I thought one would just need to adjust the loop :)

test/unit/io/sam_file/format_sam_test.cpp

test/unit/io/sam_file/sam_file_input_test.cpp

eseiler

If you feel like it, you can replace all occurrences of counter++;

LGTM, rebase + auto-merge :)

test/unit/alignment/cigar_conversion/alignment_from_cigar_test.cpp

smehringer · 2022-11-01T07:58:43Z

please do not merge yet. I think a better deprecation warning for the record is needed.

smehringer · 2022-11-01T09:04:55Z

@eseiler Can you please have a look at sam_file/record.hpp again

eseiler · 2022-11-01T11:04:54Z

include/seqan3/io/sam_file/record.hpp

-        return get_impl(field_constant<seqan3::field::alignment>{}, static_cast<tuple_base_t &&>(*this));
-    }
+    SEQAN3_DEPRECATED_340 decltype(auto) alignment() &&
+    {}


Is there a way to dispatch to alignment_from_cigar, i.e. do we get access to the reference sequence from here somehow?

no, since

It would require some dirty hacking to give extra data to the tuple (record)

there is no guarantee that a reference sequence is provided.

include/seqan3/io/sam_file/record.hpp

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 4, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from dc407de to 02023da Compare October 5, 2022 10:54

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 5, 2022

smehringer mentioned this pull request Oct 17, 2022

[IO] Change how seqan3::field::alignment works with SAM/BAM files #2907

Closed

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 7e556b6 to 4cfe87a Compare October 21, 2022 07:25

seqan-actions added the lint [INTERNAL] signal for linting label Oct 21, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 4cfe87a to c304f83 Compare October 21, 2022 08:13

seqan-actions removed the lint [INTERNAL] signal for linting label Oct 21, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 0651d20 to b138781 Compare October 21, 2022 09:31

seqan-actions added the lint [INTERNAL] signal for linting label Oct 21, 2022

eseiler removed the lint [INTERNAL] signal for linting label Oct 21, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from b138781 to 4ad6eca Compare October 21, 2022 10:18

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 21, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 0f53c40 to 92e5a5e Compare October 24, 2022 07:24

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 24, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 26fd4c4 to eee8f4a Compare October 25, 2022 09:34

seqan-actions added the lint [INTERNAL] signal for linting label Oct 25, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from eee8f4a to 056683e Compare October 25, 2022 09:36

seqan-actions removed the lint [INTERNAL] signal for linting label Oct 25, 2022

smehringer marked this pull request as ready for review October 25, 2022 11:27

smehringer requested a review from SGSSGene October 25, 2022 11:28

eseiler force-pushed the io_remove_alignment_from_sam_bam branch from 153cb15 to 4a8a14e Compare October 28, 2022 11:19

seqan-actions added the lint [INTERNAL] signal for linting label Oct 28, 2022

seqan-actions removed the lint [INTERNAL] signal for linting label Oct 28, 2022

SGSSGene approved these changes Oct 31, 2022

View reviewed changes

SGSSGene requested a review from eseiler October 31, 2022 12:59

eseiler requested changes Oct 31, 2022

View reviewed changes

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 31, 2022

smehringer requested a review from eseiler October 31, 2022 15:29

eseiler approved these changes Oct 31, 2022

View reviewed changes

eseiler reviewed Oct 31, 2022

View reviewed changes

test/unit/alignment/cigar_conversion/alignment_from_cigar_test.cpp Outdated Show resolved Hide resolved

eseiler force-pushed the io_remove_alignment_from_sam_bam branch from ce20096 to 63a9a28 Compare October 31, 2022 16:01

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 31, 2022

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 63a9a28 to 6d55996 Compare November 1, 2022 07:43

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022

[MISC] Remove field::alignment from SAM file.

f19b5cb

smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 6d55996 to f19b5cb Compare November 1, 2022 09:04

seqan-actions added the lint [INTERNAL] signal for linting label Nov 1, 2022

seqan-actions removed the lint [INTERNAL] signal for linting label Nov 1, 2022

eseiler reviewed Nov 1, 2022

View reviewed changes

Update include/seqan3/io/sam_file/record.hpp

049eb2f

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022

smehringer merged commit ad1bd5e into seqan:master Nov 1, 2022

smehringer deleted the io_remove_alignment_from_sam_bam branch November 1, 2022 12:05

smehringer mentioned this pull request Nov 1, 2022

[MISC] Remove field::offset from sam_file_[in/out]put. #3089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] Remove field::alignment from sam_file_[in/out]out. #3058

[MISC] Remove field::alignment from sam_file_[in/out]out. #3058

smehringer commented Oct 4, 2022 •

edited

Loading

vercel bot commented Oct 4, 2022 •

edited

Loading

codecov bot commented Oct 5, 2022 •

edited

Loading

smehringer commented Oct 25, 2022

eseiler Oct 31, 2022

smehringer Oct 31, 2022

eseiler Oct 31, 2022

eseiler left a comment

smehringer commented Nov 1, 2022

smehringer commented Nov 1, 2022

eseiler Nov 1, 2022

smehringer Nov 1, 2022

	for (size_t i = 0; i < 69'999; ++i)
	for (size_t i = 0; i <= 65'535; ++i)

[MISC] Remove field::alignment from sam_file_[in/out]out. #3058

[MISC] Remove field::alignment from sam_file_[in/out]out. #3058

Conversation

smehringer commented Oct 4, 2022 • edited Loading

Deprecation warning:

vercel bot commented Oct 4, 2022 • edited Loading

codecov bot commented Oct 5, 2022 • edited Loading

Codecov Report

smehringer commented Oct 25, 2022

eseiler Oct 31, 2022

Choose a reason for hiding this comment

smehringer Oct 31, 2022

Choose a reason for hiding this comment

eseiler Oct 31, 2022

Choose a reason for hiding this comment

eseiler left a comment

Choose a reason for hiding this comment

smehringer commented Nov 1, 2022

smehringer commented Nov 1, 2022

eseiler Nov 1, 2022

Choose a reason for hiding this comment

smehringer Nov 1, 2022

Choose a reason for hiding this comment

smehringer commented Oct 4, 2022 •

edited

Loading

vercel bot commented Oct 4, 2022 •

edited

Loading

codecov bot commented Oct 5, 2022 •

edited

Loading