[FEATURE] view_translate_join #1171

sarahet · 2019-07-09T14:47:58Z

This PR introduces the view_translate_join

Enables translation of two-dimensional input ranges into two-dimensional output ranges where ranges of frames per input sequence are joined
Offers potential for better performance in later applications if this is desired in comparison to standard view_translate

codecov · 2019-07-09T16:38:13Z

Codecov Report

Merging #1171 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1171      +/-   ##
==========================================
+ Coverage   96.45%   96.47%   +0.02%     
==========================================
  Files         201      202       +1     
  Lines        7752     7803      +51     
==========================================
+ Hits         7477     7528      +51     
  Misses        275      275

Impacted Files	Coverage Δ
include/seqan3/range/view/translate_join.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 0e0b73a...e2fa839. Read the comment docs.

h-2

Looks very good already, just a bunch of minor comments.

include/seqan3/range/view/translate_join.hpp

h-2 · 2019-07-09T17:50:20Z

include/seqan3/range/view/translate_join.hpp

+    //!\cond
+        requires !std::Same<remove_cvref_t<rng_t>, view_translate_join> &&
+                 std::ranges::ViewableRange<rng_t> &&
+                 std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>


Suggested change

std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>

std::Constructible<urng_t, all_view<rng_t>>

Suggested change

std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>

std::Constructible<urng_t, std::ranges::all_view<rng_t>>

include/seqan3/range/view/translate_join.hpp

test/unit/range/view/view_translate_join_test.cpp

test/unit/range/view/view_translate_test.cpp

include/seqan3/range/view/translate_join.hpp

sarahet · 2019-07-15T13:19:26Z

@h-2 Should we also have an iterator test for all nucleotide alphabets?

h-2 · 2019-07-15T13:30:12Z

include/seqan3/range/view/translate_join.hpp

+    //!\cond
+        requires !std::Same<remove_cvref_t<rng_t>, view_translate_join> &&
+                 std::ranges::ViewableRange<rng_t> &&
+                 std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>


Suggested change

std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>

std::Constructible<urng_t, std::ranges::all_view<rng_t>>

include/seqan3/range/view/translate_join.hpp

h-2 · 2019-07-15T13:34:48Z

@h-2 Should we also have an iterator test for all nucleotide alphabets?

We shouldn't need to add this here, but the simple view::translate_single should eventually get those for all nucleotides...

sarahet · 2019-07-15T13:52:25Z

I cannot comment under the suggestion for some reason ..
The suggested change std::Constructible<urng_t, std::ranges::all_view<rng_t>> does not work (as the other suggestion from above) .. this is why I did not change it in the first place when you mentioned it (not ignoring you here).

Sorry that I did not make it clearer. All your changes using all_view failed .

smehringer

Iterator template looks good :)

smehringer · 2019-07-16T05:42:48Z

include/seqan3/range/view/translate_join.hpp

+ */
+template <std::ranges::View urng_t>
+//!\cond
+    requires std::ranges::SizedRange<urng_t> &&


I guess those requirements are not needed for overload resolution but just for "type cheking"? If so we decided to use static asserts rather than concet. The error messages are more readable :)

Isn't it enough that the concepts are checked via static_assert in the adaptor? This gets anyways called when constructing the view, right?

There is a general rule in SeqAn about having static_asserts instead of requirements if they are not required for overload resolution. For the regular translate views I would argue that we might actually add other overloads and in that case the constraints for this class would be the same, but we would change the constraints on the adaptor (which would then allow e.g. BidirectionalRange but would forward those to view::translate | view::join).

TLDR: It doesn't make much of a difference. Since I don't think this particular class will get another specialisation, you can also just copy the static assert from the adaptor into the class and remove the concept constraints.

smehringer · 2019-07-16T05:46:56Z

test/unit/range/view/view_translate_join_test.cpp

+    decltype(view::translate_join(vec)) test_range = view::translate_join(vec);
+
+    template <typename A, typename B>
+    static void expect_eq(A && begin_iterator_value, B && expected_range_value)


Suggested change

static void expect_eq(A && begin_iterator_value, B && expected_range_value)

static void expect_eq(A && test_range_value, B && expected_range_value)

💅 but not neccessary

include/seqan3/range/view/translate_join.hpp

h-2 · 2019-07-18T11:44:43Z

@smehringer Do you want a final review or can I merge this after CI passes?

sarahet added the feature/proposal a new feature or an idea of label Jul 9, 2019

sarahet requested a review from h-2 July 9, 2019 14:47

sarahet assigned h-2 Jul 9, 2019

sarahet changed the title ~~Feature view translate join~~ [FEATURE] view_translate_join Jul 9, 2019

sarahet force-pushed the feature_view_translate_join branch from 910a959 to 4f240ca Compare July 9, 2019 15:25

h-2 requested changes Jul 9, 2019

View reviewed changes

sarahet force-pushed the feature_view_translate_join branch from 4f240ca to ba91828 Compare July 15, 2019 13:17

sarahet requested review from h-2 and smehringer July 15, 2019 13:18

h-2 requested changes Jul 15, 2019

View reviewed changes

sarahet force-pushed the feature_view_translate_join branch from ba91828 to 89e953d Compare July 15, 2019 13:56

sarahet requested a review from h-2 July 15, 2019 14:31

smehringer reviewed Jul 16, 2019

View reviewed changes

sarahet force-pushed the feature_view_translate_join branch from 89e953d to 53409b8 Compare July 16, 2019 17:06

sarahet requested a review from smehringer July 17, 2019 07:58

h-2 reviewed Jul 18, 2019

View reviewed changes

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

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

[FIX,DOC] Correct translated sequences in comments

2a3906e

sarahet force-pushed the feature_view_translate_join branch from 53409b8 to 285774e Compare July 18, 2019 09:01

sarahet requested a review from h-2 July 18, 2019 09:05

[FEATURE] Add view_translate_join

e2fa839

sarahet force-pushed the feature_view_translate_join branch from 285774e to e2fa839 Compare July 18, 2019 11:23

h-2 approved these changes Jul 18, 2019

View reviewed changes

h-2 merged commit 617b4b5 into seqan:master Jul 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] view_translate_join #1171

[FEATURE] view_translate_join #1171

sarahet commented Jul 9, 2019

codecov bot commented Jul 9, 2019 •

edited

Loading

h-2 left a comment

h-2 Jul 9, 2019

h-2 Jul 15, 2019

sarahet commented Jul 15, 2019

h-2 Jul 15, 2019

h-2 commented Jul 15, 2019

sarahet commented Jul 15, 2019 •

edited

Loading

smehringer left a comment

smehringer Jul 16, 2019

sarahet Jul 16, 2019 •

edited

Loading

h-2 Jul 18, 2019

smehringer Jul 16, 2019

h-2 commented Jul 18, 2019

	std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>
	std::Constructible<urng_t, all_view<rng_t>>

	std::Constructible<urng_t, ranges::ref_view<std::remove_reference_t<rng_t>>>
	std::Constructible<urng_t, std::ranges::all_view<rng_t>>

	static void expect_eq(A && begin_iterator_value, B && expected_range_value)
	static void expect_eq(A && test_range_value, B && expected_range_value)

[FEATURE] view_translate_join #1171

[FEATURE] view_translate_join #1171

Conversation

sarahet commented Jul 9, 2019

codecov bot commented Jul 9, 2019 • edited Loading

Codecov Report

h-2 left a comment

Choose a reason for hiding this comment

h-2 Jul 9, 2019

Choose a reason for hiding this comment

h-2 Jul 15, 2019

Choose a reason for hiding this comment

sarahet commented Jul 15, 2019

h-2 Jul 15, 2019

Choose a reason for hiding this comment

h-2 commented Jul 15, 2019

sarahet commented Jul 15, 2019 • edited Loading

smehringer left a comment

Choose a reason for hiding this comment

smehringer Jul 16, 2019

Choose a reason for hiding this comment

sarahet Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

h-2 Jul 18, 2019

Choose a reason for hiding this comment

smehringer Jul 16, 2019

Choose a reason for hiding this comment

h-2 commented Jul 18, 2019

codecov bot commented Jul 9, 2019 •

edited

Loading

sarahet commented Jul 15, 2019 •

edited

Loading

sarahet Jul 16, 2019 •

edited

Loading