[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

rrahn · 2020-06-04T21:50:04Z

Changes

Make strong type streamable
Add test case for exists in configuration test template
Rename the existing band to align_cfg::band_fixed_size
Introducing strong types for setting lower and upper diagonal
Direct access of the members via the config.
Adjust the tutorial and all snippets
Adjust the API documentation
Remove all references to the old static_band mechanism
Fix all dependent classes and tests

fixes seqan/product_backlog#98

marehr · 2020-06-07T17:14:39Z

I assigned myself, because I'm was also requested to review PR #1876.

marehr

Only small things! Despite the high number of changes the diff look pretty good :)

include/seqan3/core/detail/strong_type.hpp

include/seqan3/alignment/configuration/align_config_band.hpp

test/unit/alignment/configuration/align_config_band_test.cpp

test/snippet/core/algorithm/configuration_get.cpp

doc/tutorial/pairwise_alignment/index.md

marehr · 2020-06-07T17:28:03Z

include/seqan3/core/detail/strong_type.hpp

+    requires output_stream_over<stream_t, underlying_type_t const &> &&
+             ((skills & strong_type_skill::stream) != strong_type_skill::none)
+//!\endcond
+stream_t & operator<<(stream_t & stream, strong_type<underlying_type_t, derived_t, skills> const & value)


Why did you not overload debugstream?

I'm not a huge fan of overloading every possible stream operator.

I would prefer a specific overload for debugstream.

I found it weird if we say the skill allows streaming the type but then only restrict it to the debug stream. I am also in slight preference for making the strong type a public type at some point.

The question is why strong types are not always printable if the underlying type is printable?

Please just overload debug_stream 🥺

That is exactly the design of the strong types. They are not supposed to automatically inherit the behavior of the underlying type but it needs to be selected individually. If you want the type to inherit all its features just use a type alias.
It would be ok, I guess as, long as the strong type is detail to make it only debug streambale. It would not be ok if it is public. Something we should consider for the future if we are beginning to use strong types in more public interfaces. Since then the user knows what can be done with the interface. I will make it now explicit, but that should become a generic stream overload if it goes public.

rrahn · 2020-06-08T14:26:28Z

@marehr when you are done I'll rebase onto master to get rid of the changelog conflict.

marehr

🥺

marehr · 2020-06-08T18:14:59Z

include/seqan3/core/detail/strong_type.hpp

+    requires output_stream_over<stream_t, underlying_type_t const &> &&
+             ((skills & strong_type_skill::stream) != strong_type_skill::none)
+//!\endcond
+stream_t & operator<<(stream_t & stream, strong_type<underlying_type_t, derived_t, skills> const & value)


The question is why strong types are not always printable if the underlying type is printable?

Please just overload debug_stream 🥺

codecov · 2020-06-09T13:26:58Z

Codecov Report

Merging #1873 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1873   +/-   ##
=======================================
  Coverage   97.67%   97.67%           
=======================================
  Files         253      254    +1     
  Lines        9552     9565   +13     
=======================================
+ Hits         9330     9343   +13     
  Misses        222      222

Impacted Files	Coverage Δ
...ignment/matrix/detail/aligned_sequence_builder.hpp	`100.00% <ø> (ø)`
...e/seqan3/alignment/pairwise/detail/type_traits.hpp	`100.00% <ø> (ø)`
...qan3/alignment/configuration/align_config_band.hpp	`100.00% <100.00%> (ø)`
...etail/alignment_score_matrix_one_column_banded.hpp	`97.05% <100.00%> (ø)`
...trix/detail/alignment_trace_matrix_full_banded.hpp	`97.87% <100.00%> (+0.09%)`	⬆️
.../seqan3/alignment/pairwise/alignment_algorithm.hpp	`100.00% <100.00%> (ø)`
...gnment/pairwise/policy/alignment_matrix_policy.hpp	`100.00% <100.00%> (ø)`
include/seqan3/core/detail/strong_type.hpp	`100.00% <100.00%> (ø)`
...qan3/alignment/matrix/detail/matrix_coordinate.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 2248404...f63e2b9. Read the comment docs.

marehr

I'm not completely sure how to proceed.

Technically, this PR should come after #1871, but it will work any way even after that PR.

My only open issue would be renaming type_skills -> skills_.

marehr · 2020-06-09T20:03:36Z

include/seqan3/alignment/configuration/align_config_band.hpp

-    requires std::same_as<band_t, static_band>
-//!\endcond
-struct band : public pipeable_config_element<band<band_t>, band_t>
+class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>


Suggested change

class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>

class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>

Is this blocked by #1871?

No, both could be valid. But need to be updated anyway. Since #1871 is in second review already i will update this afterwards.

#1871 is merged :D

marehr · 2020-06-09T20:04:51Z

include/seqan3/core/detail/strong_type.hpp

+     */
+    { std::remove_reference_t<strong_type_t>::skills };
+    //!\cond
+    requires std::same_as<decltype(std::remove_reference_t<strong_type_t>::skills), strong_type_skill const>;


😿 This will mess up #1866 again 😿 But such is live 🤷

whatever comes first :)

marehr · 2020-06-09T20:07:33Z

include/seqan3/core/detail/strong_type.hpp

@@ -112,11 +166,12 @@ namespace seqan3::detail
 *
 * \include test/snippet/core/detail/strong_type_adding_skills.cpp
 */
-template <typename value_t, typename derived_t, strong_type_skill skills = strong_type_skill::none>
+template <typename value_t, typename derived_t, strong_type_skill type_skills = strong_type_skill::none>


Suggested change

template <typename value_t, typename derived_t, strong_type_skill type_skills = strong_type_skill::none>

template <typename value_t, typename derived_t, strong_type_skill skills_ = strong_type_skill::none>

After our new resolution 🦆 and 🏃

😢 Why did I agree on this? So ugly.

Hm I have the feeling this might be resolved in another commit but then again your commit history is clean?

my history is always clean 🙆‍♂️ :)

So you are just refusing the renaming? :D

No, but I usually sleep during the night and obviously do not update the changes then. ;)

marehr · 2020-06-09T20:13:06Z

include/seqan3/core/detail/strong_type.hpp

+SEQAN3_CONCEPT strong_type_specialisation = requires (strong_type_t && obj)
+{
+//!\endcond
+


Stupid question, but wouldn't something like this suffice?

template <typename strong_type_t> SEQAN3_CONCEPT strong_type_specialisation = requires (strong_type_t && obj) { typename strong_type_t::value_type; {strong_type_t::skills}; requires std::derived_from<strong_type_t, // I forgot the order of arguments. strong_type<typename strong_type_t::value_type, strong_type_t, strong_type_t::skills>>; }

with some additional std::remove_cvref_t for strong_type_t & instances?

I think this would be a bit more close to the intention of type specialisation. (I forgot in our earlier discussion that the strong type is inherited, so is_type_specialisation_of_v wouldn't have worked even without mixed template arguments.)

did you check?

still open?

smehringer

Only small stuff

smehringer · 2020-06-11T05:17:23Z

include/seqan3/core/detail/strong_type.hpp

+SEQAN3_CONCEPT strong_type_specialisation = requires (strong_type_t && obj)
+{
+//!\endcond
+


did you check?

include/seqan3/core/detail/strong_type.hpp

smehringer · 2020-06-11T05:20:57Z

include/seqan3/core/detail/strong_type.hpp

@@ -112,11 +166,12 @@ namespace seqan3::detail
 *
 * \include test/snippet/core/detail/strong_type_adding_skills.cpp
 */
-template <typename value_t, typename derived_t, strong_type_skill skills = strong_type_skill::none>
+template <typename value_t, typename derived_t, strong_type_skill type_skills = strong_type_skill::none>


Hm I have the feeling this might be resolved in another commit but then again your commit history is clean?

smehringer · 2020-06-11T05:22:55Z

include/seqan3/core/detail/strong_type.hpp

+ * \returns `stream_t &` A reference to the given stream.
+ */
+template <typename char_t, strong_type_specialisation strong_type_t>
+debug_stream_type<char_t> & operator<<(debug_stream_type<char_t> & stream, strong_type_t && value)


Shouldn't this also require that the underlying type is streamable?
Edit: Damn but we still have this bug that "everything is streamable" right?

include/seqan3/alignment/all.hpp

include/seqan3/alignment/configuration/align_config_band.hpp

smehringer · 2020-06-11T05:43:11Z

include/seqan3/alignment/configuration/align_config_band.hpp

-    requires std::same_as<band_t, static_band>
-//!\endcond
-struct band : public pipeable_config_element<band<band_t>, band_t>
+class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>


#1871 is merged :D

include/seqan3/alignment/configuration/align_config_band.hpp

smehringer · 2020-06-11T19:36:36Z

include/seqan3/core/detail/strong_type.hpp

+SEQAN3_CONCEPT strong_type_specialisation = requires (strong_type_t && obj)
+{
+//!\endcond
+


still open?

…gn_cfg::band_fixed_size * Rename the existing band to align_cfg::band_fixed_size * Introducing strong types for setting lower and upper diagonal * Direct access of the members via the config. * Adjust the tutorial and all snippets * Adjust the API documentation * Remove all references to the old static_band mechanism * Fix all dependent classes and tests

rrahn requested review from a team and svnbgnk and removed request for a team June 4, 2020 21:50

rrahn mentioned this pull request Jun 4, 2020

banded alignment implementation in optimised implementation #1874

Merged

marehr requested review from marehr and removed request for svnbgnk June 7, 2020 17:13

marehr requested changes Jun 7, 2020

View reviewed changes

rrahn requested a review from marehr June 8, 2020 14:26

marehr requested changes Jun 8, 2020

View reviewed changes

rrahn force-pushed the alignment/banded_implementation_config branch from b14e78b to 2595eaf Compare June 9, 2020 11:03

rrahn requested a review from marehr June 9, 2020 11:03

marehr approved these changes Jun 9, 2020

View reviewed changes

marehr requested a review from smehringer June 9, 2020 20:36

smehringer requested changes Jun 11, 2020

View reviewed changes

[TEST] Add exists method to configuration test template.

8f28cdd

rrahn force-pushed the alignment/banded_implementation_config branch from 5011d11 to 646a771 Compare June 11, 2020 12:12

rrahn requested a review from smehringer June 11, 2020 12:16

smehringer approved these changes Jun 11, 2020

View reviewed changes

rrahn added 4 commits June 11, 2020 22:36

[FEATURE] Add strong_type_specialisation concept.

db7f057

[FEATURE] Make strong type debug streamable.

29887f8

[DOC] Add changelog entry for the changed band configuration.

f63e2b9

rrahn force-pushed the alignment/banded_implementation_config branch from 646a771 to f63e2b9 Compare June 11, 2020 20:36

rrahn merged commit ba70f70 into seqan:master Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

rrahn commented Jun 4, 2020

marehr commented Jun 7, 2020

marehr left a comment •

edited

Loading

marehr Jun 7, 2020

rrahn Jun 8, 2020

marehr Jun 8, 2020

rrahn Jun 9, 2020

rrahn commented Jun 8, 2020

marehr left a comment

marehr Jun 8, 2020

codecov bot commented Jun 9, 2020 •

edited

Loading

marehr left a comment

marehr Jun 9, 2020

rrahn Jun 10, 2020

smehringer Jun 11, 2020

rrahn Jun 11, 2020

marehr Jun 9, 2020

rrahn Jun 10, 2020

marehr Jun 9, 2020

rrahn Jun 10, 2020

smehringer Jun 11, 2020

rrahn Jun 11, 2020

smehringer Jun 11, 2020

rrahn Jun 11, 2020

marehr Jun 9, 2020

rrahn Jun 10, 2020

smehringer Jun 11, 2020

smehringer Jun 11, 2020

rrahn Jun 11, 2020

smehringer left a comment

smehringer Jun 11, 2020

smehringer Jun 11, 2020

smehringer Jun 11, 2020

smehringer Jun 11, 2020

smehringer Jun 11, 2020

	class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>
	class band_fixed_size : public pipeable_config_element<band_fixed_size, seqan3::detail::empty_type>

	template <typename value_t, typename derived_t, strong_type_skill type_skills = strong_type_skill::none>
	template <typename value_t, typename derived_t, strong_type_skill skills_ = strong_type_skill::none>

[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

[API] The alignment configuration for the band changed to seqan3::align_cfg::band_fixed_size #1873

Conversation

rrahn commented Jun 4, 2020

Changes

marehr commented Jun 7, 2020

marehr left a comment • 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

Choose a reason for hiding this comment

rrahn commented Jun 8, 2020

marehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2020 • edited Loading

Codecov Report

marehr 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

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 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

marehr left a comment •

edited

Loading

codecov bot commented Jun 9, 2020 •

edited

Loading