-
Notifications
You must be signed in to change notification settings - Fork 82
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
[FIX] Iterator Tag in kmer_hash #1710
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1710 +/- ##
==========================================
+ Coverage 97.65% 97.68% +0.02%
==========================================
Files 237 237
Lines 9048 9057 +9
==========================================
+ Hits 8836 8847 +11
+ Misses 212 210 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for the view properties of kmer hash? :)
@smehringer What other tests do you want to add? There are already concept tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something small thing that caught my eye.
@@ -224,7 +224,7 @@ class kmer_hash_view<urng_t>::shape_iterator | |||
//!\brief Reference to `value_type`. | |||
using reference = value_type; | |||
//!\brief Tag this class as input iterator. | |||
using iterator_category = std::input_iterator_tag; | |||
using iterator_category = iterator_tag_t<it_t>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using iterator_category = iterator_tag_t<it_t>; | |
using iterator_category = typename std::iterator_traits<it_t>::iterator_category; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think seqan3::iterator_tag_t
was more correct.
See #853
IIRC iterators may not provide the category, at least we encountered it sometimes...
Or we somewhere down the line expect that it is there...
7a85299
to
d3c2438
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now a test dor the minimiser view? I think that does not belong here?
d3c2438
to
8c40faa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM
For the minimiser view (#1654) I had problems preserving the forward_range properties, changing the tag in the kmer hash from input range to iterator_tag solved this problem.