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

Continue analysis even when individual files fail the filtering threshold #641

Merged
merged 6 commits into from Sep 28, 2023
Merged

Continue analysis even when individual files fail the filtering threshold #641

merged 6 commits into from Sep 28, 2023

Conversation

sminot
Copy link
Contributor

@sminot sminot commented Sep 26, 2023

This PR addresses the issue that when samples fail to pass the filtering threshold, they will:

  1. Raise an error in DADA2_FILTANDTRIM when no files are written out, and
  2. Cause no outputs to be written when the id element fails to match on future join steps

I've added a test case to reproduce the bug, and the code I've added should address it.

This should be a better solution than what is outlined in #638 (which I'm now closing)

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/ampliseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@sminot sminot mentioned this pull request Sep 26, 2023
Merged
@sminot
Copy link
Contributor Author

sminot commented Sep 26, 2023

Test data in nf-core/test-datasets#1008

@sminot sminot mentioned this pull request Sep 26, 2023
Closed
10 tasks
@github-actions
Copy link

github-actions bot commented Sep 26, 2023 •
edited

nf-core lint overall result: Failed ❌

Posted for pipeline commit 8ca115e

+| ✅ 148 tests passed       |+
#| ❔   3 tests were ignored |#
!| ❗   2 tests had warnings |!
-| ❌   5 tests failed       |-

❌ Test failures:

  • files_unchanged - CODE_OF_CONDUCT.md does not match the template
  • files_unchanged - .github/CONTRIBUTING.md does not match the template
  • files_unchanged - .github/workflows/linting.yml does not match the template
  • files_unchanged - lib/NfcoreTemplate.groovy does not match the template
  • multiqc_config - 'assets/multiqc_config.yml' does not contain a matching 'report_comment'.

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.
  • schema_lint - Parameter input is not defined in the correct subschema (input_output_options)

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-09-27 17:57:10

@d4straub
Copy link
Collaborator

Thanks! That looks indeed elegant, going to test it today and give feedback.

d4straub
d4straub reviewed Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Awesome, thanks that was a good idea to change the ouput of dada2_filtntrim in that way!

The metadata however makes the test_failed fail for me, easily fixable though.

Changelog could also get an update, you can add yourself to the contributors in the readme credits section as well if you want.

subworkflows/local/dada2_preprocessing.nf Outdated Show resolved Hide resolved
conf/test_failed.config Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
@sminot
Copy link
Contributor Author

sminot commented Sep 27, 2023

Ok, I think I'm in good shape now, @d4straub

  • Test data is merged
  • Added to CI
  • Added to CHANGELOG
  • Made the channel filtering code more explicit as you suggested

Let me know what you think!

d4straub
d4straub approved these changes Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment •
edited

Choose a reason for hiding this comment

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

Great thanks, all looks good.

However, there is one more problem (apologies for not having predicted this earlier, that is a relatively new feature). For the new way of testing with github, at least a "failed.nf.test" in tests/pipeline/ seems required. That file specifies tests that output files are present, and can in conjunction with "failed.nf.test.snap" even check for md5sums. I think the presence/md5sum is only needed for central files, here probably
cutadapt/cutadapt_summary.tsv
barrnap/summary.tsv
dada2/DADA2_table.tsv
overall_summary.tsv
But I can add that also later, so I approve anyway (I think you cannot merge the PR with failing tests, let me know if you want me to merge it). However, it would be nice if you want to still add that last piece!

@sminot
Copy link
Contributor Author

sminot commented Sep 28, 2023

The test validation piece is not something I'm familiar with, so I might just vote for merging as-is and adding in those additional files in a subsequent PR.

It does appear that I cannot merge with the tests failing. If you could squash and merge that would be greatly appreciated!

Thanks also for all your help as I was getting this together @d4straub. My first real contribution to nf-core!

@d4straub d4straub merged commit 22969a7 into nf-core:dev Sep 28, 2023
15 of 17 checks passed
@sminot sminot deleted the skip_missing_data branch September 28, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@sminot @d4straub