Repository navigation
Snapshot for -profile test_single_end - #908
Conversation
-profile test_single_end
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Overall LGTM,
As a general thing, I suggest wrapping all asserts in snapshot even if it's just a boolean (you can also store these in a string even for more human readability, see nf-core/createtaxdb as an example -inspired by @maxulysse )
If that particular check fails and is not in a snapshot, it can be really annoying to debug.
I have 3 minor things that may have been missed, but otherwise this is looking very comprehensive, great work @dialvarezs !
| @@ -1,6 +1,12 @@ | |||
| .DS_Store | |||
| Ancient_DNA/pydamage/analyze/*/*.csv | |||
| Ancient_DNA/variant_calling/*/*.vcf.gz | |||
There was a problem hiding this comment.
No string check for these? Or you could use https://github.com/seppinho/nft-vcf
| .DS_Store | ||
| Ancient_DNA/pydamage/analyze/*/*.csv | ||
| Ancient_DNA/variant_calling/*/*.vcf.gz | ||
| Annotation/Prokka/**/*.{log,err,gbk,sqn} |
There was a problem hiding this comment.
No addition checks for gbk/sqn? These are common downstream files, so might be good to validate somehow
| QC_shortreads/fastqc/*_fastqc.{html,zip} | ||
| QC_shortreads/remove_phix/*.log | ||
| Taxonomy/CAT/**/*.log | ||
| VirusIdentification/geNomad/**/*_aggregated_classification.tsv |
There was a problem hiding this comment.
Can be removed, as you do actually check this explicitly in genomad_results
52543ab to
d70fc7d
Compare
|
I came back to finish this finally! @jfy133 I addressed your comments:
|
There was a problem hiding this comment.
One suggestion but not a blocker
| "Prokka GBK files contain LOCUS: ${prokka_gbk.collect { file -> path("${file}").text.contains('LOCUS') }.every()}", | ||
| "Prokka SQN files non-empty: ${prokka_sqn.collect { file -> path("${file}").size() > 0 }.every()}", | ||
| // Summary row count checks | ||
| "Bin summary non-empty: ${bin_summary.csv(sep: '\t').rowCount > 0}", |
There was a problem hiding this comment.
Could this not still be the case even if there are no hits, because you still have the header? Maybe minimum >1?
There was a problem hiding this comment.
nft-csv plugin doesn't count the header.
|
|
||
| ### `Deprecated` | ||
|
|
||
| - [#908](https://github.com/nf-core/mag/pull/908) - Removed local `quast_bins_summary` in favor of `csvtk/concat` (by @dialvarezs) |
There was a problem hiding this comment.
Note If we follow semantic versioning this requires v6 btw, as a user can not longer use a config that points to thw QUAST_BIN_SUMMARY module
There was a problem hiding this comment.
In theory yes, but I consider this one a soft one, because it's a replacement and normally users won't change the config for this (it's only a CSV concat after all).
Not like when we removed raw read taxonomic profiling, that one definitively deserved a major bump.
In dev there is also the removal of MAG_DEPTHS_PLOT, that's definitively a more serious breaking change.
But... i we adhere to strict semantic versioning, we'll end up with mag v10 on 2027 😂
Closes #902.
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).