[MISC] deprecate seqan3::field::seq_qual #2379

SGSSGene · 2021-02-14T19:59:00Z

adds [DEPRECATED] in many places.
erased mentioning of seq_qual everywhere I could find
Wrap all usages of seqan3::field::seq_qual
referenced cookbook to usage of field::seq and field::qual
referenced cookbook in changelog

vercel · 2021-02-24T12:54:38Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/8QFmD5iMT3RXQcok6VN81MPiShqG
✅ Preview: https://seqan3-git-fork-sgssgene-misc-deprecatefieldseqqual-seqan.vercel.app

codecov · 2021-02-24T13:08:32Z

Codecov Report

Merging #2379 (d7d0b07) into master (c40960b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2379   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         269      269           
  Lines       10510    10510           
=======================================
  Hits        10323    10323           
  Misses        187      187

Impacted Files	Coverage Δ
include/seqan3/io/record.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sequence_file/format_fastq.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sequence_file/input.hpp	`100.00% <ø> (ø)`
...e/seqan3/io/sequence_file/input_format_concept.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sequence_file/input_options.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sequence_file/output.hpp	`100.00% <ø> (ø)`
.../seqan3/io/sequence_file/output_format_concept.hpp	`100.00% <ø> (ø)`

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 c40960b...d7d0b07. Read the comment docs.

eaasna

Nice work!

I'm just wondering why it was decided you should avoid deprecation warnings instead of using the alternative of seqan3::views::zip and std::views::transform. Or is that a next PR?

SGSSGene · 2021-03-01T11:33:10Z

Nice work!

I'm just wondering why it was decided you should avoid deprecation warnings instead of using the alternative of seqan3::views::zip and std::views::transform. Or is that a next PR?

I am not sure what you mean by this.

I havn't planned another PR :-)

eaasna · 2021-03-01T15:38:38Z

include/seqan3/io/sequence_file/input.hpp

@@ -589,9 +592,12 @@ class sequence_file_input
    }
    //!\}

+#pragma GCC diagnostic push // avoid deprecation warning of field::seq_qual
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
    //!\brief The options are public and its members can be set directly.
    sequence_file_input_options<typename traits_type::sequence_legal_alphabet,
                             selected_field_ids::contains(field::seq_qual)> options;


Here for example the deprecated field:seq_qual is being used. It seems to me that it would make more sense to remove the usage and replace it with an alternative (since you are deprecating field:seq_qual). But you wrap it to avoid deprecation warnings instead.

Thx! Yes, this doesn't seem correct. I will look into it

Okay :) what you did does match the issue description but I also found it a bit strange

marehr · 2021-03-01T18:17:28Z

include/seqan3/io/record.hpp

@@ -64,7 +66,7 @@ enum class field
    seq,            //!< The "sequence", usually a range of nucleotides or amino acids.
    id,             //!< The identifier, usually a string.
    qual,           //!< The qualities, usually in Phred score notation.
-    seq_qual,       //!< Sequence and qualities combined in one range.
+    seq_qual SEQAN3_DEPRECATED_310, //!< [DEPRECATED] Sequence and qualities combined in one range.


I'm wondering whether

Suggested change

seq_qual SEQAN3_DEPRECATED_310, //!< [DEPRECATED] Sequence and qualities combined in one range.

_seq_qual_deprecated, //!< [DEPRECATED] Sequence and qualities combined in one range.

seq_qual SEQAN3_DEPRECATED_310 = _seq_qual_deprecated, //!< [DEPRECATED] Sequence and qualities combined in one range.

marehr · 2021-03-01T18:18:13Z

include/seqan3/io/record.hpp

@@ -108,7 +110,7 @@ enum class field
    SEQ SEQAN3_DEPRECATED_310 = seq, //!< Please use the field name in lower case.
    ID SEQAN3_DEPRECATED_310 = id, //!< Please use the field name in lower case.
    QUAL SEQAN3_DEPRECATED_310 = qual, //!< Please use the field name in lower case.
-    SEQ_QUAL SEQAN3_DEPRECATED_310 = seq_qual, //!< Please use the field name in lower case.
+    SEQ_QUAL SEQAN3_DEPRECATED_310 = seq_qual, //!< [DEPRECATED] Please use the field name in lower case.


and

Suggested change

SEQ_QUAL SEQAN3_DEPRECATED_310 = seq_qual, //!< [DEPRECATED] Please use the field name in lower case.

SEQ_QUAL SEQAN3_DEPRECATED_310 = _seq_qual_deprecated, //!< [DEPRECATED] Please use the field name in lower case.

will enable us to remove the surrondung #pragma GCC diagnostic ignored "-Wdeprecated-declarations"

I believe this is a matter of style and taste. To me both solution are valid solution.
I think that the pragma solution is very spot on, but the _seq_qal_deprecated is very compact.
You decide :-)

True, I've made the experience that opening #pragma GCC diagnostic ... can hide errors if you span it around complete classes / structs. So I would prefer a more local solution. Can you change it? :)

include/seqan3/io/sequence_file/input.hpp

marehr · 2021-03-18T19:48:26Z

Hmm something went wrong with your rebase, can you do it again?

marehr

small review, I will do the rest after the rebase was successful. I stopped for now, because I had some trouble to see what you or someone else changed.

marehr · 2021-03-18T19:43:22Z

include/seqan3/io/sequence_file/input.hpp

@@ -650,13 +647,14 @@ class sequence_file_input
        std::visit([&] (auto & f)
        {
            // read new record
-            if constexpr (selected_field_ids::contains(field::seq_qual))
+            //


Suggested change

//

// read new record

include/seqan3/io/sequence_file/input.hpp

include/seqan3/io/sequence_file/input_options.hpp

test/snippet/io/sequence_file/sequence_file_output_fields_trait_1.cpp

include/seqan3/io/sequence_file/output.hpp

test/unit/io/sequence_file/sequence_file_format_test_template.hpp

CHANGELOG.md

include/seqan3/io/sequence_file/input_options.hpp

SGSSGene · 2021-03-26T14:02:41Z

test/snippet/io/sequence_file/sequence_file_output_fields_trait_1.cpp

    seqan3::sequence_file_output fout{std::ostringstream{},
                                      seqan3::format_fasta{},
-                                      seqan3::fields<seqan3::field::id, seqan3::field::seq_qual>{}};
+                                      seqan3::fields<seqan3::field::id, seqan3::field::seq, seqan3::field::qual>{}};

    for (int i = 0; i < 5; i++)
    {
        std::string id{"test_id"};
        // vector of combined data structure:
-        std::vector<seqan3::qualified<seqan3::dna5, seqan3::phred42>> seq_qual{{'N'_dna5, '7'_phred42}};
+        std::vector<seqan3::qualified<seqan3::dna5, seqan3::phred42>> seq_qual{{'N'_dna5, '7'_phred42},
+                                                                               {'A'_dna5, '1'_phred42},
+                                                                               {'C'_dna5, '3'_phred42}};
+
+        auto view_on_seq  = seqan3::views::get<0>(seq_qual);
+        auto view_on_qual = seqan3::views::get<1>(seq_qual);

        // ...

-        fout.emplace_back(id, seq_qual);       // note also that the order the argumets is now different, because
-        // or:                                    you specified that ID should be first in the fields template argument
-        fout.push_back(std::tie(id, seq_qual));
+        fout.emplace_back(id, view_on_seq, view_on_qual); // Also note that the order of the arguments is now
+                                                          // different, because you specified that ID should be
+                                                          // first in the fields template argument
+        // or:
+        fout.push_back(std::tie(id, view_on_seq, view_on_qual));
    }


I think this snippet really confusing. So we declare an seqan3::sequence_file_output of type seqan3::format_fasta which only includes id and sequence. I explicit declare that I want to have field::qual which doesn't really mean anything, since format_fasta will drop this information?

The snippet itself talks about how the order of the arguments is now different but I actually don't know what its talking about. It is the same order as everywhere else ?!

I don't think I understand what this snippet is supposed to demonstrate. Any thoughts on this
@marehr @eseiler ?

This should be fastq output and not fasta.

The ordering refers to the default seq, id, qual which should be more clear. (2nd snippet in https://docs.seqan.de/seqan/3-master-user/tutorial_sequence_file.html#autotoc_md213 shows the default order).

Thx, I didn't get that the default order is different. Also fastq makes a lot more sense. I will change it.

eseiler · 2021-03-26T15:24:38Z

test/snippet/io/sequence_file/sequence_file_output_fields_trait_1.cpp

+        fout.emplace_back(id, view_on_seq, view_on_qual); // Also note that the order of the arguments is now
+                                                          // different, because you specified that ID should be
+                                                          // first in the fields template argument


I analysed the comment too long:

Maybe something along the lines of:

Note that the order of the arguments is different from the default `seq, id, qual`, because you specified that ID should be first in the fields template argument.

I removed the Also because this is the first thing to note.

I don't like the use of now in the sentence because there is no point of reference as to what it was before.

For the same reason, I added the default order.

I also added a period at the end :)

I think this comment could go to the line above fout.emplace_back and use the full length...

- adds [DEPRECATED] in many places. - erased mentioning of seq_qual everywhere I could find

eaasna

Looks good, I don't have anything to add :)

marehr

Thank you!

SGSSGene requested review from a team and eaasna and removed request for a team February 14, 2021 19:59

vercel bot temporarily deployed to Preview February 24, 2021 12:54 Inactive

vercel bot temporarily deployed to Preview February 24, 2021 13:45 Inactive

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 3e55de7 to 1848fcd Compare February 24, 2021 13:45

vercel bot temporarily deployed to Preview February 24, 2021 13:45 Inactive

vercel bot temporarily deployed to Preview February 25, 2021 11:49 Inactive

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from a8c6458 to 409b471 Compare February 25, 2021 12:05

vercel bot temporarily deployed to Preview February 25, 2021 12:05 Inactive

eaasna reviewed Mar 1, 2021

View reviewed changes

marehr reviewed Mar 1, 2021

View reviewed changes

marehr self-requested a review March 15, 2021 09:34

marehr reviewed Mar 15, 2021

View reviewed changes

include/seqan3/io/sequence_file/input.hpp Outdated Show resolved Hide resolved

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 409b471 to fa5dd72 Compare March 17, 2021 13:48

vercel bot temporarily deployed to Preview March 17, 2021 13:48 Inactive

SGSSGene requested a review from marehr March 17, 2021 13:49

marehr reviewed Mar 18, 2021

View reviewed changes

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from fa5dd72 to 818189e Compare March 19, 2021 11:43

vercel bot temporarily deployed to Preview March 19, 2021 11:43 Inactive

marehr reviewed Mar 19, 2021

View reviewed changes

test/snippet/io/sequence_file/sequence_file_output_fields_trait_1.cpp Outdated Show resolved Hide resolved

include/seqan3/io/sequence_file/output.hpp Show resolved Hide resolved

test/unit/io/sequence_file/sequence_file_format_test_template.hpp Outdated Show resolved Hide resolved

eseiler reviewed Mar 19, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

include/seqan3/io/sequence_file/input_options.hpp Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview March 22, 2021 10:08 Inactive

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from ffa2825 to e90f489 Compare March 23, 2021 18:51

vercel bot temporarily deployed to Preview March 23, 2021 18:51 Inactive

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from e90f489 to 25b51d7 Compare March 26, 2021 13:56

vercel bot temporarily deployed to Preview March 26, 2021 13:56 Inactive

SGSSGene commented Mar 26, 2021

View reviewed changes

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 25b51d7 to 61ee307 Compare March 26, 2021 15:14

vercel bot temporarily deployed to Preview March 26, 2021 15:14 Inactive

eseiler reviewed Mar 26, 2021

View reviewed changes

[MISC] deprecate seqan3::field::seq_qual

e81cd52

- adds [DEPRECATED] in many places. - erased mentioning of seq_qual everywhere I could find

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 61ee307 to 952fd8a Compare March 30, 2021 14:55

vercel bot temporarily deployed to Preview March 30, 2021 14:56 Inactive

SGSSGene requested a review from eaasna March 30, 2021 14:56

eaasna approved these changes Mar 31, 2021

View reviewed changes

eseiler requested a review from marehr March 31, 2021 08:58

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 952fd8a to 9e6949e Compare March 31, 2021 10:16

SGSSGene added 3 commits March 31, 2021 12:18

[MISC] split field::seq_qual in snippet

ddc503e

[MISC] update Changelog and cookbook

da96c37

[MISC] cleanup include orders

d7d0b07

SGSSGene force-pushed the misc/deprecate_field_seq_qual branch from 9e6949e to d7d0b07 Compare March 31, 2021 10:18

vercel bot temporarily deployed to Preview March 31, 2021 10:27 Inactive

marehr approved these changes Mar 31, 2021

View reviewed changes

marehr merged commit faab772 into seqan:master Mar 31, 2021

marehr mentioned this pull request Apr 12, 2021

field::seq_qual not available for Alignment IO #2101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] deprecate seqan3::field::seq_qual #2379

[MISC] deprecate seqan3::field::seq_qual #2379

SGSSGene commented Feb 14, 2021 •

edited

Loading

vercel bot commented Feb 24, 2021 •

edited

Loading

codecov bot commented Feb 24, 2021 •

edited

Loading

eaasna left a comment

SGSSGene commented Mar 1, 2021

eaasna Mar 1, 2021 •

edited

Loading

SGSSGene Mar 1, 2021

eaasna Mar 1, 2021

marehr Mar 1, 2021

marehr Mar 1, 2021

SGSSGene Mar 9, 2021

marehr Mar 15, 2021

SGSSGene Mar 17, 2021

marehr commented Mar 18, 2021

marehr left a comment

marehr Mar 18, 2021

SGSSGene Mar 26, 2021

eseiler Mar 26, 2021 •

edited

Loading

SGSSGene Mar 26, 2021

eseiler Mar 26, 2021

eaasna left a comment

marehr left a comment

	seq_qual SEQAN3_DEPRECATED_310, //!< [DEPRECATED] Sequence and qualities combined in one range.
	_seq_qual_deprecated, //!< [DEPRECATED] Sequence and qualities combined in one range.
	seq_qual SEQAN3_DEPRECATED_310 = _seq_qual_deprecated, //!< [DEPRECATED] Sequence and qualities combined in one range.

	SEQ_QUAL SEQAN3_DEPRECATED_310 = seq_qual, //!< [DEPRECATED] Please use the field name in lower case.
	SEQ_QUAL SEQAN3_DEPRECATED_310 = _seq_qual_deprecated, //!< [DEPRECATED] Please use the field name in lower case.

[MISC] deprecate seqan3::field::seq_qual #2379

[MISC] deprecate seqan3::field::seq_qual #2379

Conversation

SGSSGene commented Feb 14, 2021 • edited Loading

vercel bot commented Feb 24, 2021 • edited Loading

codecov bot commented Feb 24, 2021 • edited Loading

Codecov Report

eaasna left a comment

Choose a reason for hiding this comment

SGSSGene commented Mar 1, 2021

eaasna Mar 1, 2021 • 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

marehr commented Mar 18, 2021

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

eseiler Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eaasna left a comment

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

SGSSGene commented Feb 14, 2021 •

edited

Loading

vercel bot commented Feb 24, 2021 •

edited

Loading

codecov bot commented Feb 24, 2021 •

edited

Loading

eaasna Mar 1, 2021 •

edited

Loading

eseiler Mar 26, 2021 •

edited

Loading