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

remove ref_seq, evalue, bit_score from sam file output #2658

Merged
merged 9 commits into from
May 19, 2021

Conversation

marehr
Copy link
Member

@marehr marehr commented May 16, 2021

sam_file_output does not need the fields ref_seq, evalue, bit_score.

This commit remove those fields from the default-fields and ensures that custom fields don't contain those fields.

This commit also splits ::push_back and ::emplace_back into four versions to deprecate the usage of the old default-fields case.

@marehr marehr requested review from a team and MitraDarja and removed request for a team May 16, 2021 14:22
@vercel
Copy link

vercel bot commented May 16, 2021

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/8g2XphAh6CDkpZB3vCgiM3yGwGxS
✅ Preview: https://seqan3-git-fork-marehr-samfileoutput-seqan.vercel.app

marehr added 3 commits May 16, 2021 16:53
…_seq, evalue, bit_score})

sam_file_output does not need the fields ref_seq, evalue, bit_score.

This commit remove those fields from the default-fields and ensures
that custom fields don't contain those fields.

This commit also splits ::push_back and ::emplace_back into four
versions to deprecate the usage of the old default-fields case.
@marehr marehr force-pushed the sam_file_output branch from 5b6c6d6 to 92a5b30 Compare May 16, 2021 14:53
@marehr marehr changed the base branch from master to release-3.0.3 May 16, 2021 14:55
@codecov
Copy link

codecov bot commented May 16, 2021

Codecov Report

Merging #2658 (48cdc4b) into release-3.0.3 (bfbbb38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-3.0.3    #2658   +/-   ##
==============================================
  Coverage          98.22%   98.22%           
==============================================
  Files                276      276           
  Lines              10810    10831   +21     
==============================================
+ Hits               10618    10639   +21     
  Misses               192      192           
Impacted Files Coverage Δ
include/seqan3/io/record.hpp 100.00% <ø> (ø)
include/seqan3/io/sam_file/input.hpp 100.00% <ø> (ø)
include/seqan3/io/sam_file/output.hpp 100.00% <100.00%> (ø)
...qan3/alphabet/container/concatenated_sequences.hpp 96.89% <0.00%> (-0.02%) ⬇️
include/seqan3/io/sam_file/header.hpp 100.00% <0.00%> (ø)
include/seqan3/utility/views/slice.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/nucleotide/dna4.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/nucleotide/dna5.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/nucleotide/rna4.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/nucleotide/rna5.hpp 100.00% <0.00%> (ø)
... and 7 more

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 bfbbb38...48cdc4b. Read the comment docs.

@marehr
Copy link
Member Author

marehr commented May 17, 2021

TODO: Changelog

Copy link
Contributor

@MitraDarja MitraDarja left a comment

Choose a reason for hiding this comment

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

LGTM, one minor thing to improve doc. :)

Comment on lines 547 to 554
// new syntax enforces via static_assert that field::ref_seq, field::evalue, and field::bit_score isn't set
requires tuple_like<tuple_t> && (!detail::record_like<tuple_t>) && (!is_default_selected_field_ids)
//!\endcond
{
push_back_tuple(std::forward<tuple_t>(t));
}

// This is a bit of a problem, because to decide whether the new syntax or the old syntax is used is a bit
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments here are a bit confusing. I would suggest:

Suggested change
// new syntax enforces via static_assert that field::ref_seq, field::evalue, and field::bit_score isn't set
requires tuple_like<tuple_t> && (!detail::record_like<tuple_t>) && (!is_default_selected_field_ids)
//!\endcond
{
push_back_tuple(std::forward<tuple_t>(t));
}
// This is a bit of a problem, because to decide whether the new syntax or the old syntax is used is a bit
// The new syntax enforces via static_assert (see above) that field::ref_seq, field::evalue, and field::bit_score isn't set. This causes a problem, because it is complicated to decide whether the new syntax or the old syntax is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

@MitraDarja MitraDarja requested a review from eseiler May 17, 2021 14:21
CHANGELOG.md Outdated Show resolved Hide resolved
@eseiler eseiler enabled auto-merge (squash) May 19, 2021 12:26
@eseiler eseiler disabled auto-merge May 19, 2021 12:27
@eseiler
Copy link
Member

eseiler commented May 19, 2021

squash or rebase?

@marehr marehr merged commit fb57135 into seqan:release-3.0.3 May 19, 2021
@marehr marehr deleted the sam_file_output branch May 19, 2021 13:08
eseiler added a commit that referenced this pull request May 19, 2021
* [TEST] move debug_stream_alignment_test.cpp

* [TEST] move cigar_operation_test.cpp

* [TEST] add alphabet/range/hash_test.cpp

* [TEST] move buffer_queue_test.cpp

* [TEST] move inherited_iterator_base_test.cpp

* [TEST] move random_access_iterator_test.cpp

* [TEST] move istreambuf_view_test.cpp

* [TEST] add io/detail/magic_header_test.cpp

* [TEST] add io/detail/take_exactly_view_test.cpp

* [TEST] move char_operations_predicate_test.cpp

* [TEST] move integer_traits_test.cpp

* [TEST] move type_name_as_string_test.cpp

* [TEST] move transformation_trait_or_test.cpp

* [TEST] move convert_test.cpp

* [TEST] move deep_test.cpp

* [DOC] fix snippet

* [INFRA] update platform.hpp

* [FIX] remove unnecessary includes

* [MISC] update ranges-library and remove workaround

* [INFRA] add -DCMAKE_BUILD_TYPE=FEDORA to simulate fedora builds

* [INFRA] automatically generate snippets from a scaffold

* [DOC] generate all [rd]na(4|5|15)_implicit_conversion_from_[rd]na(4|5|15).cpp snippets

* [DOC] re-generate all [rd]na(4|5|15)(_char|)_literal

* [DOC] make sure implict conversion is as expected

* [INFRA] Ignore apt failures

* Check changelog links and add header changes. (#2641)

* Check changelog links and add header changes.

* reordering

* Add missed headers. and remove detail headers. all.hpp's are not included.

* Update CHANGELOG.md

Co-authored-by: Lydia Buntrock <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Lydia Buntrock <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Lydia Buntrock <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Lydia Buntrock <[email protected]>

* Apply 2nd review.

Co-authored-by: Lydia Buntrock <[email protected]>

* [DOC] Move changelog entries to their correct place (from 3.0.2 to 3.0.3)

* [DOC] add known good compilers to CHANGELOG.md

* [DOC] fix links in CHANGELOG.md

* [FIX] brew script

* [DOC] add documentation for CentOS 7 / RHEL 7 (#2661)

* [DOC] add documentation for CentOS 7 / RHEL 7

Fixes #2244 (comment)

* [skip ci]

* Apply suggestions from code review

Co-authored-by: Enrico Seiler <[email protected]>

Co-authored-by: Enrico Seiler <[email protected]>

* remove ref_seq, evalue, bit_score from sam file output (#2658)

* [MISC] deprecate seqan3::sam_file::output fields (seqan3::field::{ref_seq, evalue, bit_score})

sam_file_output does not need the fields ref_seq, evalue, bit_score.

This commit remove those fields from the default-fields and ensures
that custom fields don't contain those fields.

This commit also splits ::push_back and ::emplace_back into four
versions to deprecate the usage of the old default-fields case.

* [MISC] deprecate seqan3::sam_file_output::{emplace, push}_back

* [MISC] deprecate seqan3::sam_file::input fields (seqan3::field::{ref_seq, evalue, bit_score})

* [MISC] fix gcc-7 issues with SEQAN3_DEPRECATED_310

* [FIX] remove seqan3::field::ref_seq from benchmark

* [DOC] add CHANGELOG

* Apply suggestions from code review

* Apply suggestions from code review

* Update CHANGELOG.md

Co-authored-by: Enrico Seiler <[email protected]>

Co-authored-by: marehr <[email protected]>
Co-authored-by: Marcel <[email protected]>
Co-authored-by: simonsasse <[email protected]>
Co-authored-by: Lydia Buntrock <[email protected]>
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.

3 participants