[I/O] Remove column interface for sequence files #1412

joergi-w · 2019-12-10T11:16:54Z

I have to extend the record's get_or_ignore interface for common_pair, which is the result of ranges::zip with two arguments. Therefore, I use the template template tuple_like to cover both std::tuple and common_pair.

Resolves #1411.

joergi-w · 2019-12-10T14:12:04Z

Rebased on master because the CHANGELOG caused a merge conflict.

joergi-w · 2019-12-10T14:21:51Z

I had to change the solution of an alignment I/O tutorial exercise, because it was using the column interface. It is now realised with a for-loop over the file.
Additionally there was an error with the snippet output: The reference and read were both taken from index 0 of the alignment. I fixed this in the code as well as in the expected output.

codecov · 2019-12-10T15:16:52Z

Codecov Report

Merging #1412 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1412      +/-   ##
==========================================
- Coverage   97.59%   97.58%   -0.02%     
==========================================
  Files         232      232              
  Lines        8831     8783      -48     
==========================================
- Hits         8619     8571      -48     
  Misses        212      212

Impacted Files	Coverage Δ
include/seqan3/io/sequence_file/input.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/structure_file/output.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/sequence_file/output.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/detail/record.hpp	`100% <100%> (ø)`	⬆️
.../seqan3/range/container/concatenated_sequences.hpp	`96.84% <0%> (-0.02%)`	⬇️

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 36e09b3...78ab4d2. Read the comment docs.

eseiler · 2019-12-12T14:01:21Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp


-    auto mapq_filter = std::views::filter([] (auto & rec) { return get<field::MAPQ>(rec) >= 30; });
+    auto mapq_filter = std::views::filter([] (auto & rec) { return seqan3::get<seqan3::field::MAPQ>(rec) >= 30; });


This seqan3::get works?

yes, I see no error...

Apparently, but I'm wondering why :)

I guess because this get has a seqan3::record as argument...

Because this get is not a hidden friend of records

doc/tutorial/alignment_file/index.md

rrahn

Thanks for this work. Some questions and improvements that I have.

rrahn · 2019-12-16T10:46:05Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

+
+    for (auto const & record : reference_file)
+    {
+        ref_ids.push_back(seqan3::get<seqan3::field::ID>(record));


Please use std::move and capture the record by &&

rrahn · 2019-12-16T10:47:45Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

-    std::vector<std::vector<dna5>> ref_seqs = get<field::SEQ>(reference_file);
+    seqan3::sequence_file_input reference_file{tmp_dir/"reference.fasta"};
+    seqan3::concatenated_sequences<std::string> ref_ids{};
+    std::vector<std::vector<seqan3::dna5>> ref_seqs{};


Why are the sequences not stored in an concatenated set? It would make more sense here than for the ids

Yes, I swap them!

rrahn · 2019-12-16T10:50:50Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

+    seqan3::alignment_file_input mapping_file{tmp_dir/"mapping.sam",
+                                              ref_ids,
+                                              ref_seqs,
+                                              seqan3::fields<seqan3::field::ID,


Please define the fields type befofe

rrahn · 2019-12-16T10:50:50Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp


-    auto mapq_filter = std::views::filter([] (auto & rec) { return get<field::MAPQ>(rec) >= 30; });
+    auto mapq_filter = std::views::filter([] (auto & rec) { return seqan3::get<seqan3::field::MAPQ>(rec) >= 30; });


Because this get is not a hidden friend of records

rrahn · 2019-12-16T10:57:43Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

        auto & ref = get<0>(alignment);
        size_t sum_ref{};
-        std::ranges::for_each(ref.begin(), ref.end(), [&sum_ref] (auto c) { if (c == gap{}) ++sum_ref; });
+        std::ranges::for_each(ref.begin(), ref.end(), [&sum_ref] (auto c) { if (c == seqan3::gap{}) ++sum_ref; });


Use std::ranges::begin/end

Then I also use the range based for loop here.

rrahn · 2019-12-16T10:58:50Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

        size_t sum_read{};
-        std::ranges::for_each(read.begin(), read.end(), [&sum_read] (auto c) { if (c == gap{}) ++sum_read; });
+        std::ranges::for_each(read.begin(), read.end(), [&sum_read] (auto c) { if (c == seqan3::gap{}) ++sum_read; });


Please use range based for loop here

rrahn · 2019-12-16T11:02:24Z

include/seqan3/io/detail/record.hpp

@@ -135,8 +136,8 @@ auto const & get_or_ignore(record<field_types, field_ids> const & r)
 }

 //!\copydoc seqan3::detail::get_or_ignore
-template <size_t i, typename ...types>
-auto & get_or_ignore(std::tuple<types...> & t)
+template <size_t i, template <tuple_like ...types_> typename tuple_like_t, typename ...types>


So the this should be a tuple of tulles?

No it is not a tuple of tuples. The function was only valid for std::tuple as input argument and I want to extend it for seqan3::common_pair. This is needed when we call for instance

fout = seqan3::views::zip(ids, seq_quals);

Then parameter t has type seqan3::common_pair<string, vector<qualified>>.

Instead of writing two additional overloads I generalised it to tuple_like.

rrahn · 2019-12-16T11:05:23Z

include/seqan3/io/sequence_file/output.hpp

@@ -152,7 +152,7 @@ namespace seqan3
 * The record-based interface treats the file as a range of tuples (the records), but in certain situations
 * you might have the data as columns, i.e. a tuple-of-ranges, instead of range-of-tuples.
 *
- * You can use column-based writing in that case, it uses operator=() :
+ * You can use column-based writing in that case, it uses operator=() and views::zip():


Suggested change

* You can use column-based writing in that case, it uses operator=() and views::zip():

* You can use column-based writing in that case, it uses operator=() and seqan3::views::zip():

joergi-w · 2019-12-17T11:32:24Z

I rebased on master to solve merge conflicts with the fields renaming (to lowercase).

rrahn

One more change 💅 . Thank you.

rrahn · 2019-12-17T14:02:19Z

doc/tutorial/alignment_file/alignment_file_solution2.cpp

-                                                             seqan3::field::ref_id,
-                                                             seqan3::field::mapq,
-                                                             seqan3::field::alignment>{}};
+    auto const fields = seqan3::fields<seqan3::field::id,


I would only declare a type here with using. Then construct the type inplace of the constructor argument.

rrahn

lgtm

rrahn · 2019-12-18T11:58:04Z

Can you please squash the last two commits.

joergi-w · 2019-12-18T12:01:25Z

Can you please squash the last two commits.

Sure!

joergi-w requested a review from marehr December 10, 2019 11:16

joergi-w force-pushed the remove_column_interface branch from 707f7bb to 463bece Compare December 10, 2019 14:11

Irallia added a commit to Irallia/seqan3 that referenced this pull request Dec 10, 2019

[DOC] undo changes to avoid merge conflicts, will be done by seqan#1412

713a30d

Irallia mentioned this pull request Dec 10, 2019

[DOC] Remove using namespace seqan3 from tutorial #1413

Merged

Irallia added a commit to Irallia/seqan3 that referenced this pull request Dec 11, 2019

[DOC] undo changes to avoid merge conflicts, will be done by seqan#1412

14112ba

joergi-w requested review from eseiler and removed request for marehr December 12, 2019 12:02

eseiler requested changes Dec 12, 2019

View reviewed changes

eseiler approved these changes Dec 12, 2019

View reviewed changes

joergi-w requested review from smehringer and rrahn and removed request for smehringer December 12, 2019 14:27

rrahn requested changes Dec 16, 2019

View reviewed changes

joergi-w requested a review from rrahn December 17, 2019 10:55

joergi-w added 2 commits December 17, 2019 12:24

[MISC] remove the column-based interface for sequence input

f848006

[MISC] remove the column-based interface for sequence output

a934277

joergi-w force-pushed the remove_column_interface branch from 8cfb404 to 6a9752d Compare December 17, 2019 11:30

Irallia added a commit to Irallia/seqan3 that referenced this pull request Dec 17, 2019

[DOC] undo changes to avoid merge conflicts, will be done by seqan#1412

d2e2a6e

joergi-w force-pushed the remove_column_interface branch from 6a9752d to 6be8f46 Compare December 17, 2019 13:54

rrahn requested changes Dec 17, 2019

View reviewed changes

joergi-w requested a review from rrahn December 17, 2019 14:35

rrahn approved these changes Dec 18, 2019

View reviewed changes

[DOC] improve solution2 snippet of alignment file tutorial

78ab4d2

joergi-w force-pushed the remove_column_interface branch from 144b3ac to 78ab4d2 Compare December 18, 2019 12:01

rrahn merged commit 48e0461 into seqan:master Dec 19, 2019

joergi-w deleted the remove_column_interface branch March 13, 2020 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[I/O] Remove column interface for sequence files #1412

[I/O] Remove column interface for sequence files #1412

joergi-w commented Dec 10, 2019

joergi-w commented Dec 10, 2019

joergi-w commented Dec 10, 2019

codecov bot commented Dec 10, 2019 •

edited

Loading

eseiler Dec 12, 2019

joergi-w Dec 12, 2019

eseiler Dec 12, 2019

joergi-w Dec 12, 2019

rrahn Dec 16, 2019

rrahn left a comment

rrahn Dec 16, 2019

rrahn Dec 16, 2019

joergi-w Dec 17, 2019

rrahn Dec 16, 2019

rrahn Dec 16, 2019

rrahn Dec 16, 2019

joergi-w Dec 17, 2019

rrahn Dec 16, 2019

rrahn Dec 16, 2019

joergi-w Dec 17, 2019

rrahn Dec 16, 2019

joergi-w commented Dec 17, 2019

rrahn left a comment

rrahn Dec 17, 2019

rrahn left a comment

rrahn commented Dec 18, 2019

joergi-w commented Dec 18, 2019


		auto mapq_filter = std::views::filter([] (auto & rec) { return get<field::MAPQ>(rec) >= 30; });
		auto mapq_filter = std::views::filter([] (auto & rec) { return seqan3::get<seqan3::field::MAPQ>(rec) >= 30; });

	* You can use column-based writing in that case, it uses operator=() and views::zip():
	* You can use column-based writing in that case, it uses operator=() and seqan3::views::zip():

[I/O] Remove column interface for sequence files #1412

[I/O] Remove column interface for sequence files #1412

Conversation

joergi-w commented Dec 10, 2019

joergi-w commented Dec 10, 2019

joergi-w commented Dec 10, 2019

codecov bot commented Dec 10, 2019 • edited Loading

Codecov Report

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

joergi-w commented Dec 17, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

rrahn commented Dec 18, 2019

joergi-w commented Dec 18, 2019

codecov bot commented Dec 10, 2019 •

edited

Loading