Add new QC metrics and re-organized QC Message for clarity #202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
mattheww95
changed the title
emarinier
requested changes
Dec 11, 2025
| max_length = 6000000 // The maximum genome length the organism in the search field is allowed to have | ||
| max_checkm_contamination = 3.0 // The maximum level of allowed contamination allowed by CheckM | ||
| min_average_coverage = 30 // The minimum average coverage allowed | ||
| min_wgmlst_loci: The minimum number of wgMLST loci required per a sample |
| max_checkm_contamination = 3.0 // The maximum level of allowed contamination allowed by CheckM | ||
| min_average_coverage = 30 // The minimum average coverage allowed | ||
| min_wgmlst_loci: The minimum number of wgMLST loci required per a sample | ||
| min_illumina_read_length: The lowest mean illumina read length you can tolerated for your data |
Member
There was a problem hiding this comment.
capitalize Illumina, "tolerate".
Probably reword to something like: "The lowest tolerable mean Illumina read length"
| min_average_coverage = 30 // The minimum average coverage allowed | ||
| min_wgmlst_loci: The minimum number of wgMLST loci required per a sample | ||
| min_illumina_read_length: The lowest mean illumina read length you can tolerated for your data | ||
| max_illumina_read_length: The highest mean illumina read length you can tolerate for your sample |
| min_wgmlst_loci: The minimum number of wgMLST loci required per a sample | ||
| min_illumina_read_length: The lowest mean illumina read length you can tolerated for your data | ||
| max_illumina_read_length: The highest mean illumina read length you can tolerate for your sample | ||
| min_long_read_length: The minimum mean read length allowed for long read data like pacbio and nanopore |
| reisolate = 1 | ||
| resequence = 1 | ||
| failed_p = true | ||
| failed_p = false |
Member
There was a problem hiding this comment.
Is failed_p "failed probability"? Otherwise, can the p be changed to something more descriptive.
Comment on lines
470
to
+471
| def vals = [qc_data[fields[0]], qc_data[fields[1]]].sort() | ||
| if(vals[0] == null){ | ||
| if(vals[0] == null || vals[1] == null){ |
Member
There was a problem hiding this comment.
Could vals have a more descriptive name?
| "meta": { | ||
| "nf-test": "0.9.2", | ||
| "nextflow": "25.04.8" | ||
| "nextflow": "25.04.7" |
Comment on lines
+767
to
+770
| min_wgmlst_loci: The minimum number of wgMLST loci required per a sample | ||
| min_illumina_read_length: The lowest mean illumina read length you can tolerated for your data | ||
| max_illumina_read_length: The highest mean illumina read length you can tolerate for your sample | ||
| min_long_read_length: The minimum mean read length allowed for long read data like pacbio and nanopore |
Member
There was a problem hiding this comment.
Refer to comments above (near the top) about these.
| max_length = | ||
| max_checkm_contamination = 1.0 | ||
| average_coverage = | ||
| min_average_coverage = |
Member
There was a problem hiding this comment.
There's nothing after the equals sign... is that expected?
Comment on lines
+1260
to
+1261
| low_msg = "Combined mean Illumina read length is lower than expected." | ||
| high_msg = "Combined mean Illumina read length is much higher than expected." |
Member
There was a problem hiding this comment.
low says "lower", high says "much higher", I wonder if these should be symmetric, or is it intended?
sgsutcliffe
approved these changes
Dec 11, 2025
Contributor
sgsutcliffe
left a comment
sgsutcliffe
left a comment
There was a problem hiding this comment.
Sorry, nothing really to add, I just wanted to be a part of the PR. Looks good.
| max_length = null | ||
| max_checkm_contamination = 3.0 | ||
| min_average_coverage = 30 | ||
| min_illumina_read_length = 120 |
Contributor
There was a problem hiding this comment.
You want to a be more loose on the max/min? Or are these coming from somewhere specific?
| min_average_coverage = 40 | ||
| min_wgmlst_loci = 3800 | ||
| min_illumina_read_length = 120 | ||
| max_illumina_read_length = 310 |
Contributor
There was a problem hiding this comment.
I did some quick googling 300 is the max, but I still like 310 vs 300 but maybe not, but I am more concerned about a minimum of 120 because something like NovaSeq 6000 can be under 100bp or is NovaSeq not used for metagenomics?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses:
STRY0017736 - Add field and logic for
qc_status_read_lengthto mikrokondoSTRY0018931 - Implement this as a passed/failed flag for
qc_wgmlst_loci_countSTRY0019400 - Punctuation in mikrokondo pass/fail messages
"FAILED; Passed Tests: 5/6; Species ID: Escherichia coli; Organism QC Criteria: Escherichia coli"