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

[FEATURE] views::zip #2971

Merged
merged 6 commits into from
May 28, 2022
Merged

[FEATURE] views::zip #2971

merged 6 commits into from
May 28, 2022

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented May 2, 2022

Follow-up:

  • Benchmark here
  • More tests
  • Conditional explicit and noexcept specifiers for common_pair and common_tuple

@vercel
Copy link

vercel bot commented May 2, 2022

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

Name Status Preview Updated
seqan3 ✅ Ready (Inspect) Visit Preview May 28, 2022 at 1:46PM (UTC)

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #2971 (823ad77) into master (99a2299) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 823ad77 differs from pull request most recent head 8ed96cb. Consider uploading reports for the commit 8ed96cb to get more accurate results

@@            Coverage Diff             @@
##           master    #2971      +/-   ##
==========================================
+ Coverage   98.14%   98.18%   +0.03%     
==========================================
  Files         269      272       +3     
  Lines       11783    11991     +208     
==========================================
+ Hits        11565    11773     +208     
  Misses        218      218              
Impacted Files Coverage Δ
include/seqan3/utility/tuple/common_pair.hpp 100.00% <100.00%> (ø)
include/seqan3/utility/tuple/common_tuple.hpp 100.00% <100.00%> (ø)
include/seqan3/utility/views/zip.hpp 100.00% <100.00%> (ø)
...etail/alignment_matrix_column_major_range_base.hpp 100.00% <0.00%> (ø)
...matrix/detail/advanceable_alignment_coordinate.hpp 96.00% <0.00%> (+0.16%) ⬆️

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 99a2299...8ed96cb. Read the comment docs.

@eseiler
Copy link
Member Author

eseiler commented May 3, 2022

@seqan/core

Edit: Fixed by handling iterator_category

Click for previous post

I had to change some concept in the alignment code to make gcc10 work:

template <typename t>
concept indexed_sequence_pair_range =
#if defined(__GNUC__) && (__GNUC__ == 10)
    true;
#else
    std::ranges::forward_range<t> &&
    requires (std::ranges::range_value_t<t> value)
    {
        requires tuple_like<decltype(value)>;
        requires std::tuple_size_v<decltype(value)> == 2;
        requires sequence_pair<std::tuple_element_t<0, decltype(value)>>;
        requires std::copy_constructible<std::tuple_element_t<1, decltype(value)>>;
    };
#endif

This concept is quite ubiquitous in the alignment code (though it also compiles fine if you just use typename).

We also have a type trait

template <typename sequence_pairs_t>
//!\cond
    requires sequence_pair_range<std::remove_reference_t<sequence_pairs_t>>
//!\endcond
struct chunked_indexed_sequence_pairs
{
    //!\brief The transformed type that models seqan3::detail::indexed_sequence_pair_range.
    using type = decltype(views::zip(std::declval<sequence_pairs_t>(), std::views::iota(0)) | views::chunk(1));
};

In short, chunked_indexed_sequence_pairs::type should be a range over tuples.

chunked_indexed_sequence_pairs::type models indexed_sequence_pair_range before this PR on gcc10, and with this PR on gcc11+.

The type is something like views::take<ranges::subrange<views::zip... (take and subrange originate from the chunk implementation) when broken.
Here, the value type is that of views::take and this is most certainly not a tuple.

My guess is that there's something in range-v3 that "collapses" the views and returns the value type of the zip view.
With gcc11, there is more std ranges stuff, and it works there - maybe some time erasure stuff.

I would just deactivate the concept on gcc10 and revisit it once we have a chunk_view implementation.

@eseiler eseiler force-pushed the feature/zip2 branch 2 times, most recently from 2528a9e to 0e7109f Compare May 4, 2022 11:20
@eseiler eseiler force-pushed the feature/zip2 branch 2 times, most recently from 647a963 to acdcf41 Compare May 4, 2022 11:28
@eseiler eseiler marked this pull request as ready for review May 4, 2022 13:42
@eseiler eseiler changed the title [WIP] views::zip [FEATURE] views::zip May 4, 2022
@eseiler eseiler mentioned this pull request May 4, 2022
@eseiler
Copy link
Member Author

eseiler commented May 23, 2022

Because the STL says so :o I haven't looked, but I'd say partly because it would incur cost (unless static), and partly because you need it to define the view itself.

However, I'm happy to revisit the namespace stuff (maybe after the join view is in seqan3). But for now, I really want to keep it separated from the rest of seqan3.

@seqan-actions seqan-actions added the lint [INTERNAL] signal for linting label May 24, 2022
@seqan-actions seqan-actions removed the lint [INTERNAL] signal for linting label May 24, 2022
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels May 24, 2022

namespace seqan3::views
{

/*!\brief A zip view
/*!\brief A view adaptor that produces a tuple-like value of all passed views.
Copy link
Member

Choose a reason for hiding this comment

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

taken from the std documentation :)

Suggested change
/*!\brief A view adaptor that produces a tuple-like value of all passed views.
/*!\brief A view adaptor that takes several views and returns tuple-like values from every i-th element of each view.

?
I think your sentence sounds like it forms a tuple of views, not a tuple of view elements.

@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels May 28, 2022
@eseiler eseiler enabled auto-merge May 28, 2022 13:34
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels May 28, 2022
@eseiler eseiler merged commit af70359 into seqan:master May 28, 2022
@eseiler eseiler deleted the feature/zip2 branch May 28, 2022 14:09
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.

None yet

4 participants