seqan / seqan3

The modern C++ library for sequence analysis. Contains version 3 of the library and API docs.
https://www.seqan.de
Other
411 stars 82 forks source link

[MISC] Use correct comparison. #3188

Closed MitraDarja closed 1 year ago

MitraDarja commented 1 year ago

I think, we need to use "<=" as we use everywhere else ´std::less_equal´, but I could not think about an easy example where we could test the difference.

The second change I added because I am running with minions in that static assert and as I implemented the minimiser and don't really know why the statis assert is there to begin with, I was hoping, we could just delete it? :)

vercel[bot] commented 1 year ago

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

Name Status Preview Comments Updated (UTC)
seqan3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 17, 2023 1:52pm
eseiler commented 1 year ago

The <= would cause a new minimiser to be reported, even though the value didn't change:

#include <seqan3/alphabet/nucleotide/dna4.hpp>
#include <seqan3/core/debug_stream.hpp>
#include <seqan3/search/views/kmer_hash.hpp>
#include <seqan3/search/views/minimiser.hpp>

using namespace seqan3::literals;

int main()
{
    std::vector<seqan3::dna4> text{"AAAAAAAAAAAAAAA"_dna4};

    auto hashes = text | seqan3::views::kmer_hash(seqan3::shape{seqan3::ungapped{3}});
    auto minimiser = hashes | seqan3::views::minimiser(4);
    seqan3::debug_stream << minimiser << '\n';
    // with <= [0,0,0,0,0,0,0,0,0,0]
    // with <  [0,0,0]
}

The static assert would help with

#include <seqan3/core/debug_stream.hpp>
#include <seqan3/search/views/minimiser.hpp>

int main()
{
    auto minimiser = std::vector<uint64_t>(10) | seqan3::views::minimiser(3);
    seqan3::debug_stream << minimiser << '\n';
}

Though starting with a new STL version, this is allowed (GCC12 and above). GCC11 would currently work without the static_assert, because we are missing a std::forward, i.e. std::forward<urng1_t>(urange1)

https://github.com/seqan/seqan3/blob/12e3ce77f467507efec132e4af6e6328d39908de/include/seqan3/search/views/minimiser.hpp#L519

To make it then work with GCC11, we would need to replace viewable_range and views::all with the equivalents from contrib/std, which would provide the new implementation for GCC<12.