Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Oct 4, 2022

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
Copy link

vercel bot commented Oct 4, 2022

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)

@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 4, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from dc407de to 02023da Compare October 5, 2022 10:54
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 5, 2022
@codecov
Copy link

codecov bot commented Oct 5, 2022

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 smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 7e556b6 to 4cfe87a Compare October 21, 2022 07:25
@seqan-actions seqan-actions added the lint [INTERNAL] signal for linting label Oct 21, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 4cfe87a to c304f83 Compare October 21, 2022 08:13
@seqan-actions seqan-actions removed the lint [INTERNAL] signal for linting label Oct 21, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 0651d20 to b138781 Compare October 21, 2022 09:31
@seqan-actions seqan-actions added the lint [INTERNAL] signal for linting label Oct 21, 2022
@eseiler eseiler removed the lint [INTERNAL] signal for linting label Oct 21, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from b138781 to 4ad6eca Compare October 21, 2022 10:18
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 21, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 0f53c40 to 92e5a5e Compare October 24, 2022 07:24
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 24, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 26fd4c4 to eee8f4a Compare October 25, 2022 09:34
Copy link
Member

@eseiler eseiler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LGTM, rebase + auto-merge :)

@eseiler eseiler force-pushed the io_remove_alignment_from_sam_bam branch from ce20096 to 63a9a28 Compare October 31, 2022 16:01
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Oct 31, 2022
@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 63a9a28 to 6d55996 Compare November 1, 2022 07:43
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022
@smehringer
Copy link
Member Author

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

@smehringer smehringer force-pushed the io_remove_alignment_from_sam_bam branch from 6d55996 to f19b5cb Compare November 1, 2022 09:04
@seqan-actions seqan-actions added the lint [INTERNAL] signal for linting label Nov 1, 2022
@smehringer
Copy link
Member Author

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

@seqan-actions seqan-actions removed the lint [INTERNAL] signal for linting label Nov 1, 2022
return get_impl(field_constant<seqan3::field::alignment>{}, static_cast<tuple_base_t &&>(*this));
}
SEQAN3_DEPRECATED_340 decltype(auto) alignment() &&
{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, since

  1. It would require some dirty hacking to give extra data to the tuple (record)
  2. there is no guarantee that a reference sequence is provided.

include/seqan3/io/sam_file/record.hpp Outdated Show resolved Hide resolved
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022
@smehringer smehringer merged commit ad1bd5e into seqan:master Nov 1, 2022
@smehringer smehringer deleted the io_remove_alignment_from_sam_bam branch November 1, 2022 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IO] Change how seqan3::field::alignment works with SAM/BAM files
4 participants