Align config/output configuration #2024

rrahn · 2020-08-12T09:08:46Z

Replaces the align_cfg::result with the align_cfg::output.
The align_cfg::result is still needed in order to set the score type which will be fixed by another issue and is not part of this PR. So removing the result option will be done later when the score type is also changed to the new configuration.

codecov · 2020-08-12T10:02:50Z

Codecov Report

Merging #2024 into release-3.0.2 will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                Coverage Diff                @@
##           release-3.0.2    #2024      +/-   ##
=================================================
+ Coverage          97.89%   97.91%   +0.02%     
=================================================
  Files                263      263              
  Lines               9879     9887       +8     
=================================================
+ Hits                9671     9681      +10     
+ Misses               208      206       -2

Impacted Files	Coverage Δ
.../seqan3/alignment/pairwise/alignment_algorithm.hpp	`100.00% <ø> (+1.49%)`	⬆️
...e/seqan3/alignment/pairwise/detail/type_traits.hpp	`100.00% <ø> (ø)`
...de/seqan3/alignment/pairwise/edit_distance_fwd.hpp	`100.00% <ø> (ø)`
...qan3/alignment/pairwise/alignment_configurator.hpp	`100.00% <100.00%> (ø)`
...qan3/alignment/pairwise/edit_distance_unbanded.hpp	`99.25% <100.00%> (+<0.01%)`	⬆️

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 096c27e...08fcc66. Read the comment docs.

Irallia

As far as I can survey it looks very good. Above all, I have a couple of formulation suggestions.

include/seqan3/alignment/configuration/align_config_output.hpp

include/seqan3/alignment/pairwise/edit_distance_unbanded.hpp

Irallia · 2020-08-13T08:54:18Z

test/unit/alignment/configuration/align_config_common_test.cpp

    // NOTE(rrahn): You must update this test if you add a new value to seqan3::align_cfg::id
-    EXPECT_EQ(static_cast<uint8_t>(seqan3::detail::align_config_id::SIZE), 13);
+    EXPECT_EQ(static_cast<uint8_t>(seqan3::detail::align_config_id::SIZE), 19);


When does this happen? Is this part of this PR?

This happens in the first 5 commits. Each output option is a separate commit and updates this value.

Irallia

We overlooked an 'options', but I already approve.
Thanks for the PR!

include/seqan3/alignment/configuration/align_config_output.hpp

rrahn · 2020-08-14T09:08:16Z

Great, thanks. @smehringer I will first rebase and then ping you again once the commits are clean again.

rrahn · 2020-08-14T10:52:21Z

@smehringer this is now ready for second review. Thanks a lot.

rrahn · 2020-08-18T08:25:36Z

@smehringer ping pong

smehringer · 2020-08-18T10:47:08Z

include/seqan3/alignment/configuration/align_config_output.hpp

+/*!\brief Tag representing score output for the alignment algorithms.
+ * \ingroup alignment_configuration
+ */
+struct output_score_tag : public pipeable_config_element<output_score_tag>


We usually had the tag in seqan3::detail to hide it from the user?

No. In case we have a simple configuration we decided to call the class _tag and the instance of it without the tag suffix. This has nothing to do with detail or non-detail code.

In case we have a simple configuration we decided to call the class _tag and the instance of it without the tag suffix

Yes absolutely

This has nothing to do with detail or non-detail code.

I still think so, at least it is that way in all the other condifurations:
seqan3::detail::method_local_tag for seqan3::align_cfg::method_local
seqan3::detail::vectorised_tag for seqan3::align_cfg::vectorised
seqan3::detail::output_query_id_tag for seqan3::search_cfg::output_query_id
...

No 😱 they should not be in detail. Otherwise, how is the user supposed to call get on it. seqan3::get<seqan3::align_cfg::method_local> is not working and if the tag is in detail namespace, then he can't see the user documentation.

Haha well somewhere this went wrong then. I always thought that all the special configuration functionality except the | was rather for us internally anyway. Will you open an issue for this? I guess it should be in the current release then.

I will fix it right away in a separate PR.

Haha well somewhere this went wrong then.

Yes, by neither reading the user story nor the config design document. 😒

smehringer · 2020-08-18T10:56:47Z

include/seqan3/alignment/pairwise/detail/type_traits.hpp

+    static constexpr bool has_output_configuration = compute_score ||
+                                                     compute_end_positions ||
+                                                     compute_begin_positions ||
+                                                     compute_sequence_alignment;


Did you leave out align_cfg::output_sequence1_id and align_cfg::output_sequence2_id on purpose?

It is a separate issue that I will work on afterwards.

include/seqan3/alignment/pairwise/edit_distance_unbanded.hpp

smehringer · 2020-08-18T11:13:58Z

include/seqan3/alignment/pairwise/edit_distance_unbanded.hpp

-            else
-                res_vt.begin_positions = this->invalid_coordinate();
-        }
+                cached_begin_positions = alignment_begin_positions(this->trace_matrix(), cached_end_positions);


What if if constexpr (compute_end_positions) before evaluated to false and the end positions have not been properly initialised?

Then, someone configured the edit distance wrong. This is an internal dependency. I will add a static_assert to make sure that this is safe at compile time.

Edit:

What I mean is that the edit distance algorithm internally has to choose these values. These dependencies are not part of any public interface so it would be an ill-formed program by the developer and not the user. But I added the static assert to make this clear.

smehringer · 2020-08-18T11:18:09Z

include/seqan3/alignment/pairwise/edit_distance_unbanded.hpp

+            {   // Only compute begin and end position if not cached already.
                using alignment_t = decltype(res_vt.alignment);
                res_vt.alignment = alignment_trace<alignment_t>(database,
                                                                query,
                                                                this->trace_matrix(),
-                                                                res_vt.end_positions,
-                                                                res_vt.begin_positions);
+                                                                cached_end_positions,
+                                                                cached_begin_positions);
            }


I don't understand the comment in this context. Where do you compute the begin and end position "if not cashed already"? It seems you are just using the cashed_[..] variables without checking?

it is an old comment. I'll remove it

smehringer · 2020-08-18T11:27:48Z

test/unit/alignment/pairwise/align_pairwise_test.cpp

-                                    seqan3::align_cfg::result{seqan3::with_alignment, seqan3::using_score_type<double>};
+                                    seqan3::align_cfg::output_score |
+                                    seqan3::align_cfg::output_alignment |
+                                    seqan3::align_cfg::result{seqan3::with_score, seqan3::using_score_type<double>};


smehringer · 2020-08-18T11:28:00Z

test/unit/alignment/pairwise/align_result_selector_test.cpp

@@ -68,7 +78,10 @@ TEST(alignment_selector, align_result_selector_using_score_type)

    auto cfg = seqan3::align_cfg::method_global{} |
               seqan3::align_cfg::edit_scheme |
-               seqan3::align_cfg::result{seqan3::with_end_positions, seqan3::using_score_type<double>};
+               seqan3::align_cfg::result{seqan3::with_score, seqan3::using_score_type<double>} |


smehringer · 2020-08-18T11:33:20Z

include/seqan3/alignment/all.hpp

+ * aligned. For example in the global alignment the begin and end positions are identical to the positions of the
+ * original sequences since the alignment has to encompass the entire sequences, but in case of a local alignment the


Suggested change

* aligned. For example in the global alignment the begin and end positions are identical to the positions of the

* original sequences since the alignment has to encompass the entire sequences, but in case of a local alignment the

* aligned. For example, the begin and end positions of a global alignment encompass the entire sequences,

* but in case of a local alignment the

The sentence before was a little confusing and I think didn't bring in extra information.

smehringer · 2020-08-18T11:36:46Z

include/seqan3/alignment/all.hpp

+ * To obtain the actual alignment the option seqan3::align_cfg::output_alignment has to be specified.
+ * The options can be combiend with each other in order to customise the alignment and the respective output of the
+ * alignment.


Suggested change

* To obtain the actual alignment the option seqan3::align_cfg::output_alignment has to be specified.

* The options can be combiend with each other in order to customise the alignment and the respective output of the

* alignment.

* To obtain the actual alignment the option seqan3::align_cfg::output_alignment has to be specified.

* The options can be combiend with each other in order to customise the alignment algorithm and the respective output of the

How does the output customise the alignment? It can only customise the alignment algorithm, no?
I think you use "alignment" as in "the deed of aligning", but also for the resulting trace. That's a little confusing.

smehringer · 2020-08-18T11:38:14Z

include/seqan3/alignment/all.hpp

+ * If none of the above configuration was set by the user, then all output options will be enabled by default.
+ * But be aware that trying to access one of the outputs, if it has not been configured, will raise a static assertion


Hm isn't that intuitive? We compute everything but then wouldn't let people access those information?
Or did I understand this sentence wrong?

maybe a sentence is missing

smehringer · 2020-08-19T06:37:52Z

include/seqan3/alignment/configuration/align_config_output.hpp

+/*!\brief Tag representing score output for the alignment algorithms.
+ * \ingroup alignment_configuration
+ */
+struct output_score_tag : public pipeable_config_element<output_score_tag>


In case we have a simple configuration we decided to call the class _tag and the instance of it without the tag suffix

Yes absolutely

This has nothing to do with detail or non-detail code.

I still think so, at least it is that way in all the other condifurations:
seqan3::detail::method_local_tag for seqan3::align_cfg::method_local
seqan3::detail::vectorised_tag for seqan3::align_cfg::vectorised
seqan3::detail::output_query_id_tag for seqan3::search_cfg::output_query_id
...

include/seqan3/alignment/pairwise/edit_distance_unbanded.hpp

smehringer · 2020-08-19T06:43:12Z

test/unit/alignment/pairwise/align_pairwise_test.cpp

            seqan3::configuration cfg = seqan3::align_cfg::method_global{} |
                                        seqan3::align_cfg::edit_scheme |
-                                        seqan3::align_cfg::result{seqan3::with_alignment,
-                                                                  seqan3::using_score_type<double>};
+                                        seqan3::align_cfg::result{seqan3::with_score, seqan3::using_score_type<double>};


😄 well thats true. You have many TODO comments in this PR though, so I thought you deliberately did this..

Adds the new alignment configuration to output the score.

… algorithm and the configurator. This removes any dependencies to the deprecated align_cfg::result configuration, i.e. the algorithms can be ported to the new interface using the align_cfg::output configuration.

With the new align_cfg::output configuration the different features that are computed are not dependent anymore. However, some computations of the edit distance algorithm are necessary in order to compute a specific feature. For example, when the alignment is requested the edit distance algorithm must also compute the begin and end positions. This commit separates the edit distance configuration from the user configurations, i.e. if the user requested an alignment but not the begin/end positions, the algorithm still executes correctly, while at the end only the alignemnt will be stored.

This changes the alignment traits and result seclector to use the new align_cfg::output option. It changes the interface of all tests and benchmarks and snippets to use the new configuration option.

smehringer · 2020-08-19T11:24:16Z

See comment in Gitter before you merge

rrahn requested review from a team and Irallia and removed request for a team August 12, 2020 09:08

Irallia requested changes Aug 13, 2020

View reviewed changes

rrahn requested a review from Irallia August 13, 2020 14:16

Irallia approved these changes Aug 14, 2020

View reviewed changes

include/seqan3/alignment/configuration/align_config_output.hpp Outdated Show resolved Hide resolved

Irallia requested review from a team and smehringer and removed request for a team August 14, 2020 09:07

rrahn force-pushed the align_config/output_configuration branch from 20a9b26 to e8fae4e Compare August 14, 2020 10:51

rrahn requested review from smehringer and removed request for smehringer August 14, 2020 10:52

rrahn force-pushed the align_config/output_configuration branch from e8fae4e to b2bee49 Compare August 17, 2020 08:18

smehringer requested changes Aug 18, 2020

View reviewed changes

rrahn force-pushed the align_config/output_configuration branch from 8ea4bdf to 6941831 Compare August 18, 2020 15:07

rrahn requested a review from smehringer August 18, 2020 15:14

rrahn force-pushed the align_config/output_configuration branch from 6941831 to c170478 Compare August 18, 2020 16:55

smehringer requested changes Aug 19, 2020

View reviewed changes

smehringer approved these changes Aug 19, 2020

View reviewed changes

rrahn added 7 commits August 19, 2020 11:38

[FEATURE] Add seqan3::align_cfg::output_score

0558aa6

Adds the new alignment configuration to output the score.

[FEATURE] Add seqan3::align_cfg::output_end_position.

dc59b2f

[FEATURE] Add seqan3::align_cfg::output_begin_position

b09a07c

[FEATURE] Add seqan3::align_cfg::output_alignment

379e85c

[FEATURE] Add seqan3::align_cfg::output_sequence1_id

d43747b

[FEATURE] Add seqan3::align_cfg::output_sequence2_id

d2ec734

rrahn added 6 commits August 19, 2020 11:38

[TEST] Fixes the edit distance benchmark for seqan2

751f20e

[FEATURE] Enable new configuration align_cfg::output

17e0721

This changes the alignment traits and result seclector to use the new align_cfg::output option. It changes the interface of all tests and benchmarks and snippets to use the new configuration option.

[DOC] Update the module documentation.

236c98b

[DOC] Update the pairwise alignment tutorial.

5764d25

[MISC] Add API changelog entry.

08fcc66

rrahn force-pushed the align_config/output_configuration branch from 47ee83e to 08fcc66 Compare August 19, 2020 09:38

rrahn merged commit fc20e33 into seqan:release-3.0.2 Aug 19, 2020

Irallia mentioned this pull request Aug 26, 2020

[FEATURE] Alignment Configuration: Replace using_score_type tag for align_cfg::output with align_cfg::score_type #2035

Merged

rrahn deleted the align_config/output_configuration branch October 8, 2020 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align config/output configuration #2024

Align config/output configuration #2024

rrahn commented Aug 12, 2020

codecov bot commented Aug 12, 2020 •

edited

Loading

Irallia left a comment

Irallia Aug 13, 2020

rrahn Aug 13, 2020

Irallia left a comment

rrahn commented Aug 14, 2020

rrahn commented Aug 14, 2020

rrahn commented Aug 18, 2020

smehringer Aug 18, 2020

rrahn Aug 18, 2020

smehringer Aug 19, 2020

rrahn Aug 19, 2020

smehringer Aug 19, 2020

rrahn Aug 19, 2020

rrahn Aug 19, 2020

smehringer Aug 18, 2020

rrahn Aug 18, 2020

smehringer Aug 18, 2020

rrahn Aug 18, 2020 •

edited

Loading

smehringer Aug 18, 2020

rrahn Aug 18, 2020

smehringer Aug 18, 2020

rrahn Aug 18, 2020

smehringer Aug 18, 2020

smehringer Aug 18, 2020

smehringer Aug 18, 2020

smehringer Aug 18, 2020

rrahn Aug 18, 2020

smehringer Aug 19, 2020

smehringer Aug 19, 2020

smehringer commented Aug 19, 2020

		* aligned. For example in the global alignment the begin and end positions are identical to the positions of the
		* original sequences since the alignment has to encompass the entire sequences, but in case of a local alignment the

		* If none of the above configuration was set by the user, then all output options will be enabled by default.
		* But be aware that trying to access one of the outputs, if it has not been configured, will raise a static assertion

Align config/output configuration #2024

Align config/output configuration #2024

Conversation

rrahn commented Aug 12, 2020

codecov bot commented Aug 12, 2020 • edited Loading

Codecov Report

Irallia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irallia left a comment

Choose a reason for hiding this comment

rrahn commented Aug 14, 2020

rrahn commented Aug 14, 2020

rrahn commented Aug 18, 2020

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

rrahn Aug 18, 2020 • 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

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 commented Aug 19, 2020

codecov bot commented Aug 12, 2020 •

edited

Loading

rrahn Aug 18, 2020 •

edited

Loading