[FEATURE] Alphabet: Add the quality alphabet seqan3::phred94 #2290

Irallia · 2020-12-01T16:01:56Z

The task 'Check if the tutorials need an update.' did not require any work, because there is no tutorial yet. I will add a tutorial when solving seqan/product_backlog#266.
For task 'Update the documentation on the quality landing page' I left three comments for the reviewer, which I will remove after discussion.

I splittet the PR in several commits and would recommend to review commit wise.

codecov · 2020-12-01T16:32:59Z

Codecov Report

Merging #2290 (d797403) into master (066b704) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2290   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         260      261    +1     
  Lines       10809    10820   +11     
=======================================
+ Hits        10613    10624   +11     
  Misses        196      196

Impacted Files	Coverage Δ
include/seqan3/alphabet/quality/phred42.hpp	`100.00% <ø> (ø)`
include/seqan3/alphabet/quality/phred63.hpp	`100.00% <ø> (ø)`
include/seqan3/alphabet/quality/phred94.hpp	`100.00% <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 066b704...d797403. Read the comment docs.

smehringer · 2020-12-02T14:26:13Z

doc/tutorial/sequence_file/index.md

@@ -186,7 +186,7 @@ which by default is set to seqan3::sequence_file_input_default_traits_dna.
 We thereby assume that
 * you want to read the sequence into a std::vector over a seqan3::dna5 alphabet,
 * store the sequence names/id in a std::string and
-* the qualities in a std::vector over the seqan3::phred42 alphabet.
+* the qualities in a std::vector over the seqan3::phred42 alphabet. <- da default phred94 werden soll, ändert sich hier etwas?


ja aber das wird am besten in einem separatem PR gemacht :)

smehringer · 2020-12-02T14:27:46Z

include/seqan3/alphabet/quality/all.hpp

+ * | Sanger, Illumina 1.8+ short | seqan3::phred42       | [0 .. 41]         | [0 .. 41]  | ['!' .. 'J'] | Phred score in [0 .. 61]  |
+ * | Sanger, Illumina 1.8+ long  | seqan3::phred63       | [0 .. 62]         | [0 .. 62]  | ['!' .. '_'] | Phred score in [0 .. 62]  |
+ * | Solexa, Illumina [1.0; 1.8[ | seqan3::phred68legacy | [-5 .. 62]        | [0 .. 67]  | [';' .. '~'] | Phred score in [-5 .. 62] |
+ * | PacBio                      | seqan3::phred94       | [0 .. 62]         | [0 .. 93]  | ['!' .. '~'] | Phred score in [0 .. 93]  |


This is not a Pacbio format. It is actually also Sanger, Illumina 1.8+ but the full range. The other two are both short in that sense

should I rename long into middle when I rename PacBio into Sanger, Illumina 1.8+ full range?

smehringer · 2020-12-02T14:30:59Z

test/unit/alphabet/composite/composite_integration_test.cpp

@@ -25,14 +26,15 @@ using gapped_qualified_dna_phred42 = seqan3::gapped<qualified_dna_phred42>;
 using qualified_qualified_gapped_dna_phred42_phred42 = seqan3::qualified<qualified_gapped_dna_phred42, seqan3::phred42>;
 using gapped_alphabet_variant_dna_phred42 = seqan3::gapped<seqan3::alphabet_variant<seqan3::dna4, seqan3::phred42>>;

-// Some haessllihckeiten-tests
+// Some haessllihckeiten-tests <--???


🙈 that was frustrated-me when I worked on this 2 years ago.
The review process clearly failed here 🤣

Didn't we already remove this in another PR? Are there multiple occurrences?

MitraDarja

Looks already really good, thanks for the advice how to review that helped a lot. I only have some small things... :)
I could not answer every question, so I would leave that to the second reviewer.

MitraDarja · 2020-12-03T12:34:10Z

include/seqan3/alphabet/quality/phred94.hpp

+ * ['!' .. '~']. It represents the PacBio standard typically HiFi reads. Beyond these the typical range of Sanger,
+ * Solexa or Illumina raw reads is (0 to 41), namely seqan3::phred42, and the larger score type (0 to 62), namely


Can you change the sentence to something more like: For other (more typical) phred scores see...?
The Beyond start throw me off my reading. 😅

MitraDarja · 2020-12-03T12:38:09Z

test/snippet/alphabet/quality/phred94.cpp

+    seqan3::debug_stream << another_phred.to_phred() << "\n"; // 75
+    // we need to cast to (int) for human readable console output


But we don't cast here to int? Also, if you write a complete sentence in a comment, I would use a dot in the end and capitalize the first word.

MitraDarja · 2020-12-03T12:38:58Z

test/snippet/alphabet/quality/phred94.cpp

+    seqan3::debug_stream << static_cast<int>(phred.to_phred()) << "\n"; // 2
+    seqan3::debug_stream << phred.to_char() << "\n";        // '#'
+    seqan3::debug_stream << static_cast<int>(phred.to_rank()) << "\n";  // 2


Is to_rank and to_phred the same?

In this case, yes. But not for every phred score alphabet, see seqan3::phred68legacy. When you have a look in the Table of the all.hpp you will recognize that the score could be negative.

MitraDarja · 2020-12-03T12:40:27Z

test/unit/alphabet/composite/composite_integration_test.cpp

@@ -25,14 +26,15 @@ using gapped_qualified_dna_phred42 = seqan3::gapped<qualified_dna_phred42>;
 using qualified_qualified_gapped_dna_phred42_phred42 = seqan3::qualified<qualified_gapped_dna_phred42, seqan3::phred42>;
 using gapped_alphabet_variant_dna_phred42 = seqan3::gapped<seqan3::alphabet_variant<seqan3::dna4, seqan3::phred42>>;

-// Some haessllihckeiten-tests
+// Some haessllihckeiten-tests <--???


MitraDarja · 2020-12-03T12:47:48Z

include/seqan3/alphabet/quality/all.hpp

 * An overview of all the score formats and their encodings can be found here:
- * https://en.wikipedia.org/wiki/FASTQ_format#Encoding.
+ * https://en.wikipedia.org/wiki/FASTQ_format#Encoding. <-- letztes abrufdatum hinzufügen? (last access 01.12.2020)


I think adding the last access date is good practice, so you should do it.

MitraDarja · 2020-12-03T12:48:57Z

include/seqan3/alphabet/quality/all.hpp

 *
- * The most distributed format is the *Sanger* or <I>Illumina 1.8+</I> format.
+ * The most distributed format is the *Sanger* or <I>Illumina 1.8+</I> format. <-- ist Sanger noch most dirstributed? Und dann nicht eher PacBio?


I think it still holds true.

Irallia · 2020-12-04T14:00:07Z

@MitraDarja After you approve, I will rebase and remove the changes for the test/snippet/alphabet/quality/phred63.cpp and put them into a seperate PR, because you where totaly right, we dont need to cast to int so I will remove these things for the other snippets aswell.
I made also a bigger change for the table in the all.hpp. I hope that makes everything a bit clearer. I will adjust the text in the documentation PR for seqan/product_backlog#266.
For test/unit/alphabet/composite/composite_integration_test.cpp I spoke with @smehringer and we decided not to add any tests here, because a combination with phred::94 is the same as a combination with phred::63.

MitraDarja

Almost done, just some really small things. ;)
Can you also check why the CI fails?

test/snippet/alphabet/quality/phred94.cpp

MitraDarja · 2020-12-04T14:13:20Z

test/unit/alphabet/composite/composite_integration_test.cpp

+// tests various combinations of alphabet_variant and alphabet_tuple
+


Remove the empty line?

MitraDarja

Thanks. :)

smehringer

Great work :)

Only some small 💅 of some documentation phrasing and one question.

smehringer · 2020-12-07T12:28:28Z

CHANGELOG.md

+* Added `seqan3::phred94`, a quality type for PacBio Phred scores of HiFi reads
+  ([\#2290](https://github.com/seqan/seqan3/pull/2290)).


Suggested change

* Added `seqan3::phred94`, a quality type for PacBio Phred scores of HiFi reads

([\#2290](https://github.com/seqan/seqan3/pull/2290)).

* Added `seqan3::phred94`, a quality type that represents the full Phred Score range (Sanger format)

and is used for PacBio Phred scores of HiFi reads ([\#2290](https://github.com/seqan/seqan3/pull/2290)).

smehringer · 2020-12-07T12:32:16Z

include/seqan3/alphabet/quality/all.hpp

+ * If you want to store PacBio HiFi reads, we recommend to use seqan3::phred93, as these reads are stored in SAM/BAM
+ * format and thus use the full range of the phred quality scores.


Suggested change

* If you want to store PacBio HiFi reads, we recommend to use seqan3::phred93, as these reads are stored in SAM/BAM

* format and thus use the full range of the phred quality scores.

* If you want to store PacBio HiFi reads, we recommend to use seqan3::phred94, as these

* use the full range of the phred quality scores.

Illumina is also stored in SAM/BAM so this might be confusing :)

smehringer · 2020-12-07T12:35:03Z

include/seqan3/alphabet/quality/phred63.hpp

@@ -21,7 +21,7 @@
 namespace seqan3
 {

-/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores (full range).
+/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores (full range). <-- ist 93 nicht full range?


Suggested change

/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores (full range). <-- ist 93 nicht full range?

/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores.

I would just remove it for now and you can decide in the "Improve Docu PR" if you add something here again that is more helpful than "full range" or "middle range"

smehringer · 2020-12-07T12:37:34Z

include/seqan3/alphabet/quality/phred94.hpp

+// ------------------------------------------------------------------
+// phred94
+// ------------------------------------------------------------------
+


Suggested change

// ------------------------------------------------------------------

// phred94

// ------------------------------------------------------------------

Since there is nothing else in this file we don't need to add such separations I think :)

smehringer · 2020-12-07T12:40:50Z

include/seqan3/alphabet/quality/phred94.hpp

+ * \details
+ *
+ * The phred94 quality alphabet represents the zero-based phred score range [0..93] mapped to the ASCII range
+ * ['!' .. '~'] (Sanger, Illumina 1.8+ format). It represents the PacBio use case, typically HiFi reads. For Sanger and


Suggested change

* ['!' .. '~'] (Sanger, Illumina 1.8+ format). It represents the PacBio use case, typically HiFi reads. For Sanger and

* ['!' .. '~'] (Sanger, Illumina 1.8+ format). It is typically used for HiFi reads produced by PacBio. For Sanger and

smehringer · 2020-12-07T12:45:15Z

include/seqan3/alphabet/quality/phred94.hpp

+ * Illumina phred scores of raw reads the range is (0 to 41), represented as seqan3::phred42, and the larger score type
+ * (0 to 62), represented as seqan3::phred63.


Suggested change

* Illumina phred scores of raw reads the range is (0 to 41), represented as seqan3::phred42, and the larger score type

* (0 to 62), represented as seqan3::phred63.

* Illumina phred scores of raw reads the range is typically (0 to 41), represented as seqan3::phred42. If you expect only slightly

* larger score types you can use seqan3::phred63 (0 to 62) which still has memory advantages when used with `seqan3::qualified`.

check line length after applying the suggestion.

smehringer · 2020-12-07T12:48:48Z

test/snippet/alphabet/quality/phred94.cpp

+    seqan3::phred94 another_phred{75};
+    seqan3::debug_stream << another_phred.to_phred() << "\n"; // 75
+}


What happens when you construct a phred of a higher value than 93 ? e.g. 105.
Can you check what happens?

smehringer · 2020-12-07T12:49:25Z

test/snippet/alphabet/quality/phred94_literal.cpp

+#include <seqan3/alphabet/quality/phred94.hpp>
+#include <seqan3/core/debug_stream.hpp>
+#include <seqan3/std/algorithm>


Suggested change

#include <seqan3/alphabet/quality/phred94.hpp>

#include <seqan3/core/debug_stream.hpp>

#include <seqan3/std/algorithm>

#include <seqan3/std/algorithm>

#include <seqan3/alphabet/quality/phred94.hpp>

#include <seqan3/core/debug_stream.hpp>

smehringer · 2020-12-08T07:58:46Z

Please clean the commit history if you want the commits to remain separate. Otherwise ping me to (sqaush) merge.

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

… test cases Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia self-assigned this Dec 1, 2020

Irallia requested review from a team and MitraDarja and removed request for a team December 2, 2020 08:45

smehringer reviewed Dec 2, 2020

View reviewed changes

MitraDarja requested changes Dec 3, 2020

View reviewed changes

Irallia requested a review from MitraDarja December 4, 2020 13:54

MitraDarja requested changes Dec 4, 2020

View reviewed changes

Irallia mentioned this pull request Dec 4, 2020

[DOC] Refactor phred snippets. The int cast is not needed. #2291

Merged

Irallia requested a review from MitraDarja December 5, 2020 23:43

MitraDarja approved these changes Dec 7, 2020

View reviewed changes

MitraDarja requested review from a team and smehringer and removed request for a team December 7, 2020 08:13

smehringer requested changes Dec 7, 2020

View reviewed changes

smehringer approved these changes Dec 8, 2020

View reviewed changes

Irallia added 5 commits December 8, 2020 10:54

[FEATURE] Add the quality alphabet seqan3::phred94

4d2c8d1

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[DOC, TEST] Correct the phred42 and phred63 documentation, expand the…

6b794ae

… test cases Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[TEST] Add phred94 to the performance / benchmark tests.

4d2389b

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[DOC] Approve documentation

fc7a2f2

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[DOC] Changelog entry added

d797403

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia force-pushed the feature/alphabet/add_phred94 branch from 1d16c0b to d797403 Compare December 8, 2020 10:00

smehringer merged commit c812a00 into seqan:master Dec 8, 2020

Irallia mentioned this pull request May 7, 2021

[MISC] Alignment: aminoacid scoring scheme - lower case member constants #2599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Alphabet: Add the quality alphabet seqan3::phred94 #2290

[FEATURE] Alphabet: Add the quality alphabet seqan3::phred94 #2290

Irallia commented Dec 1, 2020

codecov bot commented Dec 1, 2020 •

edited

smehringer Dec 2, 2020

smehringer Dec 2, 2020

Irallia Dec 4, 2020

smehringer Dec 2, 2020

eseiler Dec 2, 2020

MitraDarja Dec 3, 2020

MitraDarja left a comment

MitraDarja Dec 3, 2020

MitraDarja Dec 3, 2020

MitraDarja Dec 3, 2020

Irallia Dec 4, 2020

MitraDarja Dec 3, 2020

MitraDarja Dec 3, 2020

MitraDarja Dec 3, 2020

Irallia commented Dec 4, 2020 •

edited

MitraDarja left a comment

MitraDarja Dec 4, 2020

MitraDarja left a comment

smehringer left a comment

smehringer Dec 7, 2020

smehringer Dec 7, 2020 •

edited by eseiler

smehringer Dec 7, 2020

smehringer Dec 7, 2020

smehringer Dec 7, 2020

smehringer Dec 7, 2020

smehringer Dec 7, 2020

smehringer Dec 7, 2020

smehringer commented Dec 8, 2020

		* ['!' .. '~']. It represents the PacBio standard typically HiFi reads. Beyond these the typical range of Sanger,
		* Solexa or Illumina raw reads is (0 to 41), namely seqan3::phred42, and the larger score type (0 to 62), namely

		seqan3::debug_stream << another_phred.to_phred() << "\n"; // 75
		// we need to cast to (int) for human readable console output

		// tests various combinations of alphabet_variant and alphabet_tuple

		* Added `seqan3::phred94`, a quality type for PacBio Phred scores of HiFi reads
		([\#2290](https://github.com/seqan/seqan3/pull/2290)).

		* If you want to store PacBio HiFi reads, we recommend to use seqan3::phred93, as these reads are stored in SAM/BAM
		* format and thus use the full range of the phred quality scores.

	/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores (full range). <-- ist 93 nicht full range?
	/*!\brief Quality type for traditional Sanger and modern Illumina Phred scores.

	// ------------------------------------------------------------------
	// phred94
	// ------------------------------------------------------------------

	* ['!' .. '~'] (Sanger, Illumina 1.8+ format). It represents the PacBio use case, typically HiFi reads. For Sanger and
	* ['!' .. '~'] (Sanger, Illumina 1.8+ format). It is typically used for HiFi reads produced by PacBio. For Sanger and

		* Illumina phred scores of raw reads the range is (0 to 41), represented as seqan3::phred42, and the larger score type
		* (0 to 62), represented as seqan3::phred63.

[FEATURE] Alphabet: Add the quality alphabet seqan3::phred94 #2290

[FEATURE] Alphabet: Add the quality alphabet seqan3::phred94 #2290

Conversation

Irallia commented Dec 1, 2020

codecov bot commented Dec 1, 2020 • edited

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

Choose a reason for hiding this comment

MitraDarja 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

Irallia commented Dec 4, 2020 • edited

MitraDarja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitraDarja left a 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

smehringer Dec 7, 2020 • edited by eseiler

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 Dec 8, 2020

codecov bot commented Dec 1, 2020 •

edited

Irallia commented Dec 4, 2020 •

edited

smehringer Dec 7, 2020 •

edited by eseiler