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

Search confguration/on result release #2019

Merged

Conversation

rrahn
Copy link
Contributor

@rrahn rrahn commented Aug 10, 2020

Supersedes #2012
fixes seqan/product_backlog#169

@rrahn rrahn requested a review from eseiler August 10, 2020 13:54
@rrahn rrahn force-pushed the search_confguration/on_result_release branch from 8dcff4b to 41cdc10 Compare August 10, 2020 13:56
@rrahn rrahn mentioned this pull request Aug 10, 2020
Copy link
Member

@eseiler eseiler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(you can change the branch to merge against via the edit button near the PR name)

Can you also add our semiregular_box stuff to the view_take_until?

@rrahn
Copy link
Contributor Author

rrahn commented Aug 10, 2020

(you can change the branch to merge against via the edit button near the PR name)

I know, but I accidently rebased some stuff from master locally and so I decided to make a clean branch from the release branch.

@rrahn
Copy link
Contributor Author

rrahn commented Aug 10, 2020

Can you also add our semiregular_box stuff to the view_take_until?

I'll grep for it and update all occurrences.

@rrahn rrahn force-pushed the search_confguration/on_result_release branch from 41cdc10 to 205ecaa Compare August 10, 2020 14:28
@rrahn rrahn requested a review from smehringer August 10, 2020 14:28
@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #2019 into release-3.0.2 will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-3.0.2    #2019      +/-   ##
=================================================
- Coverage          97.90%   97.89%   -0.01%     
=================================================
  Files                262      263       +1     
  Lines               9863     9879      +16     
=================================================
+ Hits                9656     9671      +15     
- Misses               207      208       +1     
Impacted Files Coverage Δ
...alignment/configuration/align_config_on_result.hpp 100.00% <ø> (ø)
include/seqan3/range/views/take_until.hpp 98.24% <ø> (ø)
include/seqan3/search/configuration/on_result.hpp 100.00% <100.00%> (ø)
include/seqan3/search/detail/policy_max_error.hpp 100.00% <100.00%> (ø)
...an3/search/detail/policy_search_result_builder.hpp 100.00% <100.00%> (ø)
...e/seqan3/search/detail/search_scheme_algorithm.hpp 94.53% <100.00%> (-0.03%) ⬇️
.../search/detail/unidirectional_search_algorithm.hpp 100.00% <100.00%> (ø)
include/seqan3/search/search.hpp 100.00% <100.00%> (ø)
...de/seqan3/argument_parser/detail/version_check.hpp 92.74% <0.00%> (-0.81%) ⬇️
include/seqan3/core/algorithm/configuration.hpp 100.00% <0.00%> (ø)
... and 2 more

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 6ddb906...473a8eb. Read the comment docs.

Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, Only very small things. You can directly rebase

@@ -92,6 +93,9 @@ struct search_traits
output_reference_id |
output_reference_begin_position |
output_index_cursor;

//!\brief A flag indicating whether a user provided callback was given.
static constexpr bool is_one_way_execution = search_configuration_t::template exists<search_cfg::on_result>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused with the naming. "Two-way execution" I first got to know when you introduced it in the context of executors/parallelization (or do I remember that wrongly?). Now you use one-way execution for a user defined callback? Can we maybe rather use something like has_user_defined_callback?

EDIT: I see below that you use it to differentiate between executors. Can you change the brief then? If the user provides a callback, we need to use a one_way_execution...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the point. In the executor context two-way means that it returns something to the user he can wait for and one-way means it does not return anything but just executes it and the user needs to make sure the code is handled properly. But I see that the term might be confusing and I will rename it.

*
* In the default case, a call to seqan3::search returns a lazy range over the results of the search. This lazy range
* has the advantage that the results are always in a deterministic order even if the search is executed in parallel.
* Sometimes, however, it might be challenging to provide a user defined callback.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Sometimes, however, it might be challenging to provide a user defined callback.
* Sometimes, however, it might be preferred to provide a user defined callback.

? Why should it be challenging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ask @eseiler 😏 I had demanding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

demanding in this context means difficult; but obviously you meant preferred, I just didn't catch your meaning :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, what I actually meant was desirable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 desirable it is

@rrahn rrahn requested a review from smehringer August 11, 2020 12:02
The algorithms are initialsied during construction with the configuration object.
This commit removes the superfluous copy of the
configuration object and changes the
initialisation of the policies accordingly.
This configuration element can be used to provide a user defined callback to the search algorithm.
Later when we configure the result type we need a way to avoid default instantiation of the configuration object due to the
semiregularbox needed to store the callback.
This version makes use of the get_or interface and using
a dedicated empty search result type definition.
Enables the second execution path of the search when on_result was specified.
The search becomes a void function and bulk_executes the algorithm with the given user callback.
@rrahn rrahn force-pushed the search_confguration/on_result_release branch from 714aaf3 to 473a8eb Compare August 12, 2020 10:36
@rrahn rrahn merged commit 1bbd1eb into seqan:release-3.0.2 Aug 12, 2020
@rrahn rrahn deleted the search_confguration/on_result_release branch October 8, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants