[FEATURE] Forward call of indexed sequence pairs to alignment algorithm. #1358

rrahn · 2019-11-05T15:09:41Z

codecov · 2019-11-05T16:22:35Z

Codecov Report

Merging #1358 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
+ Coverage   97.48%   97.49%   +<.01%     
==========================================
  Files         218      218              
  Lines        8680     8689       +9     
==========================================
+ Hits         8462     8471       +9     
  Misses        218      218

Impacted Files	Coverage Δ
...airwise/execution/execution_handler_sequential.hpp	`100% <100%> (ø)`	⬆️
.../pairwise/execution/execution_handler_parallel.hpp	`100% <100%> (ø)`	⬆️

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 e83bfef...25d9152. Read the comment docs.

eseiler

LGTM

Is the parallel handler's execute tested somewhere?

rrahn · 2019-11-05T20:28:53Z

@eseiler the parallel handler is tested. But you are right I did not add a concrete test for this, but I should I guess within the tasks we said not to test yet. But I guess here I can.

rrahn · 2019-11-05T22:50:59Z

@eseiler now it is tested 😉

smehringer

mostly questions

smehringer · 2019-11-07T07:52:41Z

include/seqan3/alignment/pairwise/execution/execution_handler_parallel.hpp

+     * \param[in] delegate               A callable which will be invoked on each result of the computed alignments.
+     */
+    template <typename algorithm_t, indexed_sequence_pair_range indexed_sequence_pairs_t, typename delegate_type>
+    void execute(algorithm_t && algorithm,


Isn't this always a specialisation of seqan3::alignment_algorithm? Or is this a policy?
Sorry for always picking on your types, but I always stumble about this when trying to understand what's happening.

I also do not understand what a delegate is (for).. 🙈

this is the algorithm that is supposed to be called by the execution handler. That can be basically anything that is invocable, so it was called fn_t for function before. Since it is only used in the context of executing alignment algorithms it is now refined to be a more specific in the name.

Then maybe add the comment, that it must model callable ?

And what is the delegate thing ?

Then maybe add the comment, that it must model callable ?

I will have another invocable concept for this after all the open PRs with these changes are mereged. Then I can make sure that this works. Is that ok?

Ah and a delegate is another callable that is called on the result of the alignment algorithm. The term is a standard term for something that should be called on the result of something without jumping out of the process (as opposed to coroutines). https://stackoverflow.com/questions/9568150/what-is-a-c-delegate

I will have another invocable concept for this after all the open PRs with these changes are mereged. Then I can make sure that this works. Is that ok?

Sure.

Ah and a delegate is another callable that is called on the result of the alignment algorithm. The term is a standard term for something that should be called on the result of something without jumping out of the process (as opposed to coroutines). https://stackoverflow.com/questions/9568150/what-is-a-c-delegate

Ok, and what is the purpose of this here/what is the use case?

We have two interfaces for dealing with alignment results. The first one results a alignment result range and the second calls a user defined delegate on the result. Accordingly, there are two executors. The two way which we already know and the one way which will not implement the buffering. So both of them reuse the execution handlers which just get a delegate to call the alignment result on. The two way just passes a lambda that moves the result into the correct buffer location.

smehringer · 2019-11-07T07:59:14Z

include/seqan3/alignment/pairwise/execution/execution_handler_parallel.hpp

+     * \tparam delegate_type            The type of the callable invoked on the std::invoke_result of `algorithm_t`.
+     *
+     * \param[in] algorithm              The alignment algorithm to invoke.
+     * \param[in] indexed_sequence_pairs The range of underlying annotated sequence pairs to be aligned.


You say range here but copy. Do you assume that this copy is cheap? Is it a view?

Ok, so it expects always a range.
It never uses the range as a lvalue reference because it executes the alignments asynchronously and want's to make sure that another thread is not modifying this reference. And in the implementation we actually pass in a rvalue view so that nothing is expensive in this context.

Ok. If you always pass a view in our context we might want to constrain this?
But no strong feelings

Well, the interface does not care really. It just takes a copy of whatever comes in. Just we use it by passing in a view which is lightweight. The only constraint I might add here is that the type is move constructible since afterwards we move it around.

smehringer · 2019-11-07T08:01:13Z

test/unit/alignment/pairwise/execution/execution_handler_template.hpp

+
+TYPED_TEST_CASE_P(execution_handler);
+
+TYPED_TEST_P(execution_handler, execute_w_lvalue)


what is a w_lvalue?

EDIT: Do you mean with? 😆 please be expressive in the tests 😇

yes it is supposed to read with.

rrahn requested a review from eseiler November 5, 2019 15:09

eseiler approved these changes Nov 5, 2019

View reviewed changes

rrahn requested a review from smehringer November 5, 2019 20:27

rrahn force-pushed the feature/range_interface_for_parallel_execution_handler branch from 04f7143 to df7729b Compare November 5, 2019 22:49

smehringer requested changes Nov 7, 2019

View reviewed changes

rrahn added 2 commits November 7, 2019 16:20

[FEATURE] Forward call of indexed sequence pairs to alignment algorithm.

12ba7a6

[TEST] Add test for range interface in execution handler.

25d9152

rrahn force-pushed the feature/range_interface_for_parallel_execution_handler branch from df7729b to 25d9152 Compare November 7, 2019 15:20

rrahn requested a review from smehringer November 7, 2019 15:20

smehringer approved these changes Nov 8, 2019

View reviewed changes

rrahn requested a review from smehringer November 8, 2019 09:38

smehringer approved these changes Nov 12, 2019

View reviewed changes

smehringer merged commit ca6dd02 into seqan:master Nov 12, 2019

rrahn deleted the feature/range_interface_for_parallel_execution_handler branch January 16, 2020 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Forward call of indexed sequence pairs to alignment algorithm. #1358

[FEATURE] Forward call of indexed sequence pairs to alignment algorithm. #1358

rrahn commented Nov 5, 2019

codecov bot commented Nov 5, 2019 •

edited

Loading

eseiler left a comment

rrahn commented Nov 5, 2019

rrahn commented Nov 5, 2019

smehringer left a comment

smehringer Nov 7, 2019

rrahn Nov 7, 2019

smehringer Nov 7, 2019

smehringer Nov 7, 2019

rrahn Nov 7, 2019

smehringer Nov 8, 2019

rrahn Nov 8, 2019

smehringer Nov 7, 2019

rrahn Nov 7, 2019

smehringer Nov 7, 2019 •

edited

Loading

rrahn Nov 7, 2019

smehringer Nov 7, 2019

rrahn Nov 7, 2019


		TYPED_TEST_CASE_P(execution_handler);

		TYPED_TEST_P(execution_handler, execute_w_lvalue)

[FEATURE] Forward call of indexed sequence pairs to alignment algorithm. #1358

[FEATURE] Forward call of indexed sequence pairs to alignment algorithm. #1358

Conversation

rrahn commented Nov 5, 2019

codecov bot commented Nov 5, 2019 • edited Loading

Codecov Report

eseiler left a comment

Choose a reason for hiding this comment

rrahn commented Nov 5, 2019

rrahn commented Nov 5, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer Nov 7, 2019 • edited Loading

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

codecov bot commented Nov 5, 2019 •

edited

Loading

smehringer Nov 7, 2019 •

edited

Loading