Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Metagenomic end #158

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Metagenomic end #158

wants to merge 8 commits into from

Conversation

mattheww95
Copy link
Collaborator

Added option to prevent metagenomic samples from going through additional downstream processing.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 19, 2025 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 00b2780

+| ✅ 228 tests passed       |+
#| ❔  32 tests were ignored |#
!| ❗  82 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • files_exist - File is ignored: CODE_OF_CONDUCT.md
  • files_exist - File is ignored: assets/nf-core-mikrokondo_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-mikrokondo_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-mikrokondo_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: docs/output.md
  • files_exist - File is ignored: docs/README.md
  • files_exist - File is ignored: docs/usage.md
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • nextflow_config - Config variable ignored: params.max_cpus
  • files_unchanged - File does not exist: CODE_OF_CONDUCT.md
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File does not exist: .github/ISSUE_TEMPLATE/config.yml
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/feature_request.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-mikrokondo_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-mikrokondo_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-mikrokondo_logo_dark.png
  • files_unchanged - File does not exist: docs/README.md
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/mikrokondo/mikrokondo/.github/workflows/awstest.yml
  • multiqc_config - multiqc_config

✅ Tests passed:

Run details

  • nf-core/tools version 3.2.0
  • Run at 2025-03-20 19:59:25

@mattheww95
Copy link
Collaborator Author

Latest tests with nextflow fail due to an issue in ScriptLoaderV2. Links to the further issue can be found here: nextflow-io/nextflow#5896

@mattheww95
Copy link
Collaborator Author

This PR addresses the story: STRY0017074

@mattheww95 mattheww95 marked this pull request as ready for review March 19, 2025 20:47
emarinier
emarinier requested changes Mar 20, 2025
View reviewed changes
Copy link
Member

@emarinier emarinier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. My suggestion would be adding another test for when a sample is metagenomic and the user specifies fail_on_metagenomic = false, unless it's implicitly covered by another test?

subworkflows/local/clean_reads.nf Outdated

// Check if samples are contaminated/metagenomic and save the user computational time
if (params.fail_on_metagenomic){
temp_channel = ch_cleaned_reads.branch{ v, fastq ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the biggest fan of temp_channel as a name, but I also don't have any suggestions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other thing I can think of is intermediate_channel but that is just more typing.

sgsutcliffe
sgsutcliffe approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@sgsutcliffe sgsutcliffe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

sgsutcliffe
sgsutcliffe reviewed Mar 20, 2025
View reviewed changes
subworkflows/local/clean_reads.nf Outdated

// Check if samples are contaminated/metagenomic and save the user computational time
if (params.fail_on_metagenomic){
temp_channel = ch_cleaned_reads.branch{ v, fastq ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 'v' ? I didn't get a chance to look at what the channel looks like to see what v was. Might be helpful to be descriptive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in: 86f8630

@mattheww95
Copy link
Collaborator Author

mattheww95 commented Mar 20, 2025 •
edited
Loading

It looks good to me. My suggestion would be adding another test for when a sample is metagenomic and the user specifies fail_on_metagenomic = false, unless it's implicitly covered by another test?

This test is covered! There is test running just a basic metagenomic assembly that passes.

The relevant tests are here:

mikrokondo/tests/main.nf.test

Lines 526 to 611 in 86f8630

test("Should run without failure metagenomic.") {
tag "succeed_assembly_meta"
when {
params {
input = "https://raw.githubusercontent.com/phac-nml/mikrokondo/dev/tests/data/samplesheets/samplesheet-small-assembly.csv"
outdir = "results"
platform = "illumina"
mash_sketch = "https://github.com/phac-nml/mikrokondo/raw/dev/tests/data/databases/campy-staph-ecoli.msh"
mh_min_kmer = 1
dehosting_idx = "https://github.com/phac-nml/mikrokondo/raw/dev/tests/data/databases/campy.mmi"
kraken2_db = "$baseDir/tests/data/kraken2/test"
metagenomic_run = true
min_reads = 100
skip_allele_calling = true
skip_bakta = true
skip_staramr = false
skip_mobrecon = false
skip_checkm = false
skip_raw_read_metrics = false
skip_polishing = false
max_memory = "2.GB"
max_cpus = 1
}
}
then {
assert workflow.success
assert path("$launchDir/results").exists()
// parse output json file
def iridanext_json = path("$launchDir/results/iridanext.output.json").json
assert iridanext_json.metadata.samples.short."QC Status" == "NA"
assert iridanext_json.metadata.samples.short."QC Summary" == "No quality control criteria is applied for metagenomic samples."
}
}
test("Should run without failure metagenomic viruses.") {
tag "succeed_assembly_meta_viruses"
when {
params {
input = "$baseDir/tests/data/samplesheets/samplesheet-small-metagenomic.csv"
outdir = "results"
platform = "illumina"
mash_sketch = "$baseDir/tests/data/databases/virus.msh"
mh_min_kmer = 1
dehosting_idx = "https://github.com/phac-nml/mikrokondo/raw/dev/tests/data/databases/campy.mmi"
kraken2_db = "$baseDir/tests/data/kraken2/test"
min_reads = 2
skip_allele_calling = true
skip_bakta = true
skip_staramr = false
skip_mobrecon = false
skip_checkm = false
skip_raw_read_metrics = false
skip_polishing = false
max_memory = "2.GB"
max_cpus = 1
}
}
then {
assert workflow.success
assert path("$launchDir/results").exists()
// parse output json file
def iridanext_json = path("$launchDir/results/iridanext.output.json").json
assert iridanext_json.metadata.samples."meta-small"."QC Status" == "FAILED"
assert iridanext_json.metadata.samples."meta-small"."QC Summary" == "[FAILED] Sample was determined to be metagenomic and this was not specified as a metagenomic run indicating contamination."
}
}

@mattheww95
Copy link
Collaborator Author

It looks good to me. My suggestion would be adding another test for when a sample is metagenomic and the user specifies fail_on_metagenomic = false, unless it's implicitly covered by another test?

Added explicit test parameter: 00b2780

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgsutcliffe sgsutcliffe sgsutcliffe approved these changes

@emarinier emarinier emarinier approved these changes

@apetkau apetkau Awaiting requested review from apetkau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattheww95 @emarinier @sgsutcliffe