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

kmer_hash does not preserve sized_range properties correctly #1719

Closed
MitraDarja opened this issue Apr 7, 2020 · 4 comments
Closed

kmer_hash does not preserve sized_range properties correctly #1719

MitraDarja opened this issue Apr 7, 2020 · 4 comments
Labels
bug faulty or wrong behaviour of code ready to tackle Fully specified, discussed and understood. Can be tackled.
Milestone

Comments

@MitraDarja
Copy link
Contributor

MitraDarja commented Apr 7, 2020

When the underlying range is std::list while using kmer_hash the sized_range property is not preserved as it should be.

// According to kmer_view sized_range properties is preserved and for other type of ranges (e.g. std::vector) it is true, but not for std::list???
    std::list<seqan3::dna4> text5{'A'_dna4, 'C'_dna4, 'G'_dna4, 'T'_dna4, 'A'_dna4, 'G'_dna4, 'C'_dna4};
    auto v5 = text5 | ungapped_view;
    seqan3::debug_stream << std::ranges::sized_range<decltype(text5)>; //prints: 1
    seqan3::debug_stream << std::ranges::sized_range<decltype(v5)>;    // prints: 0
@MitraDarja MitraDarja added the bug faulty or wrong behaviour of code label Apr 7, 2020
@rrahn
Copy link
Contributor

rrahn commented Apr 7, 2020

Ok the problem is the following. All our views inherit from std::ranges::view_interface. This adds member functions based on some pre-conditions. For example it adds a size member function if the sentinel and the iterator model sized_sentinel_for. But there are 2 different ways to check if a range is a sized range. Either it offers a size member function which returns the size in constant time, or if this is not given it checks if the range has sized_sentinel_for sentinel,iterator pair. Since it would be false to just use size of ther unterlying range for the view interface it uses sized_sentinel_for condition to check if the ranges is sized. This not true for lists anymore.

To fix this:

  • add an explpict size member function to the kmer_hash view which is only enabled if the underlying range is sized
  • the size member function returns std::ranges::size(urng) - shape.size() + 1
  • test that the size is 0 if the underlying range is empty or the shape is larger than the underlying range
  • add a changelog entry

@rrahn rrahn modified the milestones: Sprint, Sprint 2 Apr 7, 2020
@rrahn rrahn added the ready to tackle Fully specified, discussed and understood. Can be tackled. label Apr 7, 2020
@marehr marehr changed the title Kmer_Hash does not preserve sized_range properties correctly kmer_hash does not preserve sized_range properties correctly Apr 7, 2020
@MitraDarja
Copy link
Contributor Author

MitraDarja commented Apr 7, 2020

@rrahn Because the bug appeared in the test cases I edited (#1722), I would like try to fix it, should I wait with it until the next iteration? Plus can I include the fix in the mentioned PR or should I create a separate one?

I just added the fix to my PR. 😃

@rrahn
Copy link
Contributor

rrahn commented Apr 15, 2020

Thanks for working on this. In general I would make it a separate PR in teh future. But for now this is ok.

@rrahn
Copy link
Contributor

rrahn commented May 12, 2020

fixed by #1722

@rrahn rrahn closed this as completed May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug faulty or wrong behaviour of code ready to tackle Fully specified, discussed and understood. Can be tackled.
Projects
None yet
Development

No branches or pull requests

2 participants