[FEATURE] shape, shape_iterator and view::kmer_hash #946

eseiler · 2019-05-03T09:45:22Z

Supersedes #922

Await judgement by codecov

codecov · 2019-05-03T18:49:08Z

Codecov Report

Merging #946 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
- Coverage   96.86%   96.85%   -0.01%     
==========================================
  Files         217      218       +1     
  Lines        8664     8746      +82     
==========================================
+ Hits         8392     8471      +79     
- Misses        272      275       +3

Impacted Files	Coverage Δ
include/seqan3/range/view/kmer_hash.hpp	`100% <100%> (ø)`	⬆️
include/seqan3/search/kmer_index/shape.hpp	`100% <100%> (ø)`
include/seqan3/range/container/small_vector.hpp	`99.25% <100%> (ø)`	⬆️
...de/seqan3/range/detail/inherited_iterator_base.hpp	`89.28% <0%> (-10.72%)`	⬇️

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 11d176b...a2f8d4d. Read the comment docs.

smehringer

what do the performance tests say?

include/seqan3/search/kmer_index/shape.hpp

smehringer · 2019-05-06T07:45:22Z

include/seqan3/search/kmer_index/shape.hpp

+    */
+    template <typename... t>
+    //!\cond
+        requires sizeof...(t) >= 2 && sizeof...(t) <= 32 && (std::Integral<t> && ...)


Suggested change

requires sizeof...(t) >= 2 && sizeof...(t) <= 32 && (std::Integral<t> && ...)

requires sizeof...(t) >= 2 && sizeof...(t) <= 32 && (std::Same<t, bool> && ...)

?

but using shape s{1, 0, 1, 1} deduces the type to int, hence the check fails. I could do ImplicitlyConvertibleTo but nearly everything is convertible to bool.

ah I see.. well then it is fine I guess

Then please add an assert that every value is either 1 or 0!

Alternatively or additionally you might want to add support for initialising from a binary literal, e.g. 0b1101

yeah, the binary literal thing will conflict with the length constructor....

include/seqan3/search/kmer_index/shape_iterator.hpp

eseiler · 2019-05-06T13:17:59Z

Edit: It's not really bp/s what I'm measuring, but it's comparable within one test case (e.g. all gapped ones are comparable with the other gapped ones)/

Benchmark (name/sequence_length/k)	Throughput[Bp/s]	Time	CPU	Iterations
seqan2_ungapped/1000/8	338.809M/s	3044 ns	2952 ns	228884
shape_iterator_ungapped/1000/8	361.079M/s	2781 ns	2769 ns	238861
seqan2_ungapped/1000/32	344.308M/s	2989 ns	2904 ns	235807
shape_iterator_ungapped/1000/32	364.317M/s	2758 ns	2745 ns	243567
seqan2_ungapped/50000/8	332.346M/s	169627 ns	150446 ns	4601
shape_iterator_ungapped/50000/8	357.357M/s	140485 ns	139916 ns	4645
seqan2_ungapped/50000/32	353.45M/s	142501 ns	141463 ns	4428
shape_iterator_ungapped/50000/32	354.364M/s	142111 ns	141098 ns	4805
seqan2_gapped/1000/8	338.198M/s	2967 ns	2957 ns	219057
shape_iterator_gapped/1000/8	134.621M/s	7476 ns	7428 ns	92921
seqan2_gapped/1000/32	115.799M/s	8682 ns	8636 ns	69363
shape_iterator_gapped/1000/32	45.703M/s	21987 ns	21880 ns	30026
seqan2_gapped/50000/8	335.232M/s	149748 ns	149150 ns	4448
shape_iterator_gapped/50000/8	131.69M/s	383638 ns	379679 ns	1797
seqan2_gapped/50000/32	111.697M/s	450866 ns	447640 ns	1582
shape_iterator_gapped/50000/32	45.3863M/s	1105704 ns	1101654 ns	615
shape_iterator_ungapped_random_access/1000/8	164.162M/s	61225 ns	60915 ns	11910
shape_iterator_ungapped_random_access/1000/32	46.9264M/s	214028 ns	213100 ns	3143
shape_iterator_ungapped_random_access/50000/8	158.698M/s	63233 ns	63013 ns	10814
shape_iterator_ungapped_random_access/50000/32	47.0508M/s	213453 ns	212536 ns	2809
shape_iterator_gapped_random_access/1000/8	169.638M/s	59281 ns	58949 ns	12044
shape_iterator_gapped_random_access/1000/32	53.3727M/s	188106 ns	187362 ns	3185
shape_iterator_gapped_random_access/50000/8	165.014M/s	60950 ns	60601 ns	10914
shape_iterator_gapped_random_access/50000/32	51.8345M/s	193616 ns	192922 ns	3386

smehringer

One more thing: We usually out all public members first and then all private members. You have something like public, private, public in the shape_iterator.

include/seqan3/search/kmer_index/shape_iterator.hpp

smehringer

In general the shape iterator will go into a view right?
Then it should probably inherit from inherited_iterator_base? I think this will e.g. already take care that your iterator is comparable to the inherited iterator (you tempplate argument)

include/seqan3/search/kmer_index/all.hpp

include/seqan3/search/kmer_index/shape.hpp

include/seqan3/search/kmer_index/shape_iterator.hpp

eseiler · 2019-05-09T13:36:50Z

I'll have a look at the inherited_iterator_base

include/seqan3/search/kmer_index/shape.hpp

include/seqan3/search/kmer_index/shape_iterator.hpp

test/snippet/search/kmer_index/shape_iterator.cpp

test/unit/search/kmer_index/shape_test.cpp

eseiler · 2019-05-15T13:34:27Z

I didn't know where to put the old view::kmer_hash, so I put it in test/include/performance/std_kmer_hash.hpp for now.

Benchmark[seq_len/k]	Throughput[bp/s]	Time	CPU	Iterations
seqan_kmer_hash/1000/8	348.089M/s	2867 ns	2853 ns	243795
seqan_kmer_hash/1000/32	325.167M/s	2993 ns	2980 ns	238183
seqan_kmer_hash/50000/8	355.52M/s	141154 ns	140619 ns	4845
seqan_kmer_hash/50000/32	353.035M/s	142342 ns	141541 ns	4687
std_kmer_hash/1000/8	241.554M/s	4127 ns	4111 ns	163673
std_kmer_hash/1000/32	63.5777M/s	15299 ns	15241 ns	44485
std_kmer_hash/50000/8	232.499M/s	216047 ns	215025 ns	3253
std_kmer_hash/50000/32	63.3054M/s	792137 ns	789332 ns	870

smehringer

Do we ever need the shape oterator without the view?

include/seqan3/range/view/kmer_hash.hpp

include/seqan3/search/kmer_index/detail/shape_iterator.hpp

smehringer · 2019-05-16T08:13:26Z

include/seqan3/search/kmer_index/detail/shape_iterator.hpp

+    /*!\brief Move the iterator to a given position and return the corresponding hash value.
+     * \attention This function is only avaible if it_t models std::RandomAccessIterator.
+     */
+    value_type operator[](std::make_unsigned_t<difference_type> const n)


Suggested change

value_type operator[](std::make_unsigned_t<difference_type> const n)

value_type operator[](size_type const n)

?

Probably yes, but it depends on the passed iterator type; so I'd leave it as is?

include/seqan3/search/kmer_index/detail/shape_iterator.hpp

eseiler · 2019-08-16T11:38:55Z

@h-2 I switched to the dynamic_bitset. Can you have another look>

include/seqan3/search/kmer_index/shape.hpp

include/seqan3/range/view/kmer_hash.hpp

h-2

We are getting there!

include/seqan3/range/view/kmer_hash.hpp

h-2 · 2019-08-22T16:50:01Z

include/seqan3/range/view/kmer_hash.hpp

+
+// ---------------------------------------------------------------------------------------------------------------------
+// kmer_hash_fn (adaptor definition)
+// ---------------------------------------------------------------------------------------------------------------------

 //![adaptor_def]


include/seqan3/range/view/kmer_hash.hpp

h-2 · 2019-08-26T13:08:40Z

include/seqan3/range/view/kmer_hash.hpp

+using iterator          = shape_iterator<urng_t>;
+//!\brief The const_iterator type of this view.
+using const_iterator    = shape_iterator<urng_t const>;
+//!\}


wrong indention

Do you use these at all? You have return type auto below...

auto begin() { return iterator{...}; }

I changed it to

iterator begin() { return {...}; }

h-2 · 2019-08-26T13:13:06Z

include/seqan3/range/view/kmer_hash.hpp

+
+// ---------------------------------------------------------------------------------------------------------------------
+// kmer_hash_fn (adaptor definition)
+// ---------------------------------------------------------------------------------------------------------------------

 //![adaptor_def]


The place where this is included, reads :

This is the full proto-adaptor, first look at the second member function: it handles range and argument input and returns a range (in other cases this would delegate to the view's constructor if you have your own view type declared).

This needs to be adapted, or not?

eseiler mentioned this pull request May 3, 2019

[FEATURE] shape and shape_iterator #922

Closed

eseiler force-pushed the feature/shape branch from d97d82a to 8e7e2bb Compare May 3, 2019 18:49

eseiler force-pushed the feature/shape branch from 8e7e2bb to 57272fa Compare May 3, 2019 20:44

smehringer requested changes May 6, 2019

View reviewed changes

eseiler force-pushed the feature/shape branch from 57272fa to a586d32 Compare May 6, 2019 08:57

eseiler force-pushed the feature/shape branch 2 times, most recently from 39cdbef to 50cc5aa Compare May 6, 2019 13:24

smehringer requested changes May 8, 2019

View reviewed changes

smehringer requested changes May 9, 2019

View reviewed changes

eseiler force-pushed the feature/shape branch from 50cc5aa to 4a2d896 Compare May 9, 2019 13:36

eseiler force-pushed the feature/shape branch 5 times, most recently from ad567cf to bf97b33 Compare May 10, 2019 08:13

smehringer requested changes May 10, 2019

View reviewed changes

eseiler changed the title ~~[FEATURE] shape and shape_iterator~~ [FEATURE] shape, shape_iterator and view::kmer_hash May 14, 2019

eseiler force-pushed the feature/shape branch from 6ea9af4 to e8d711c Compare May 14, 2019 18:10

eseiler requested a review from smehringer May 14, 2019 18:12

eseiler force-pushed the feature/shape branch from e8d711c to e3b43d5 Compare May 15, 2019 13:36

smehringer requested changes May 16, 2019

View reviewed changes

eseiler force-pushed the feature/shape branch from e3b43d5 to 3ae61dd Compare May 16, 2019 13:13

eseiler requested a review from smehringer May 16, 2019 13:13

eseiler force-pushed the feature/shape branch 2 times, most recently from c0fc1e5 to 0beae3a Compare May 17, 2019 11:55

eseiler force-pushed the feature/shape branch 2 times, most recently from 623aa02 to fb16fe6 Compare June 19, 2019 13:56

eseiler requested a review from h-2 June 21, 2019 05:21

h-2 removed their request for review June 27, 2019 11:42

eseiler force-pushed the feature/shape branch 2 times, most recently from c455d8c to 95ab0c2 Compare August 16, 2019 11:35

eseiler requested a review from h-2 August 16, 2019 11:38

eseiler commented Aug 16, 2019

View reviewed changes

include/seqan3/search/kmer_index/shape.hpp Outdated Show resolved Hide resolved

eseiler force-pushed the feature/shape branch 2 times, most recently from a27c9bb to c46cdbb Compare August 16, 2019 12:54

eseiler commented Aug 16, 2019

View reviewed changes

include/seqan3/range/view/kmer_hash.hpp Outdated Show resolved Hide resolved

h-2 requested changes Aug 22, 2019

View reviewed changes

eseiler force-pushed the feature/shape branch from c46cdbb to a7b196c Compare August 23, 2019 09:33

eseiler requested a review from h-2 August 23, 2019 12:00

eseiler force-pushed the feature/shape branch from a7b196c to 37c0d3f Compare August 26, 2019 13:01

h-2 requested changes Aug 26, 2019

View reviewed changes

eseiler force-pushed the feature/shape branch from 37c0d3f to 7eaa172 Compare August 26, 2019 13:26

eseiler requested a review from h-2 August 26, 2019 13:28

eseiler force-pushed the feature/shape branch from 7eaa172 to 2e97e96 Compare August 26, 2019 13:39

eseiler added 2 commits August 29, 2019 11:04

[MISC] Add assert

0db280f

[FEATURE] shape

874d883

eseiler force-pushed the feature/shape branch 2 times, most recently from ecae70e to d14b87e Compare August 29, 2019 09:11

[FEATURE] new kmer_hash view

a2f8d4d

eseiler force-pushed the feature/shape branch from d14b87e to a2f8d4d Compare August 29, 2019 11:25

h-2 approved these changes Aug 29, 2019

View reviewed changes

h-2 merged commit aa28a13 into seqan:master Aug 29, 2019

eseiler deleted the feature/shape branch August 29, 2019 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] shape, shape_iterator and view::kmer_hash #946

[FEATURE] shape, shape_iterator and view::kmer_hash #946

eseiler commented May 3, 2019 •

edited

Loading

codecov bot commented May 3, 2019 •

edited

Loading

smehringer left a comment

smehringer May 6, 2019

eseiler May 6, 2019

smehringer May 6, 2019

h-2 May 20, 2019

h-2 May 20, 2019

h-2 May 28, 2019

eseiler commented May 6, 2019 •

edited

Loading

smehringer left a comment

smehringer left a comment

eseiler commented May 9, 2019

eseiler commented May 15, 2019 •

edited

Loading

smehringer left a comment

smehringer May 16, 2019

eseiler May 16, 2019

eseiler commented Aug 16, 2019

h-2 left a comment

h-2 Aug 22, 2019

h-2 Aug 26, 2019

h-2 Aug 26, 2019

eseiler Aug 26, 2019

h-2 Aug 26, 2019

	requires sizeof...(t) >= 2 && sizeof...(t) <= 32 && (std::Integral<t> && ...)
	requires sizeof...(t) >= 2 && sizeof...(t) <= 32 && (std::Same<t, bool> && ...)

	value_type operator[](std::make_unsigned_t<difference_type> const n)
	value_type operator[](size_type const n)

[FEATURE] shape, shape_iterator and view::kmer_hash #946

[FEATURE] shape, shape_iterator and view::kmer_hash #946

Conversation

eseiler commented May 3, 2019 • edited Loading

codecov bot commented May 3, 2019 • edited Loading

Codecov Report

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented May 6, 2019 • edited Loading

smehringer left a comment

Choose a reason for hiding this comment

smehringer left a comment

Choose a reason for hiding this comment

eseiler commented May 9, 2019

eseiler commented May 15, 2019 • edited Loading

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented Aug 16, 2019

h-2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented May 3, 2019 •

edited

Loading

codecov bot commented May 3, 2019 •

edited

Loading

eseiler commented May 6, 2019 •

edited

Loading

eseiler commented May 15, 2019 •

edited

Loading