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.

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

Add nonpareil #417

Merged
merged 14 commits into from
Nov 30, 2023
Merged

Add nonpareil #417

merged 14 commits into from
Nov 30, 2023

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented Nov 13, 2023

Add nonpareil metagenome redundancy estimation

Includes non-standard MultiQC output: the tool using R to generate a extrapolation plot.

The output from this is either a PNG or a R S3/S4 object that stores the raw data. There is no way to export the 'extrapolated' data as it is calculated in the plotting function. Therefore it's not really possible to add to MultiQC as otherwise it would require reconstructing the R function in python, and would involve computation in the MultiQC step.

Instead I've gone with adding a custom MultiQC section that embeds the PNG in the plot.

Example report here:
multiqc.zip

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/taxprofiler 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).

@jfy133 jfy133 marked this pull request as draft November 13, 2023 11:01
@jfy133

This comment was marked as outdated.

Copy link

github-actions bot commented Nov 13, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 40d1bfa

+| ✅ 159 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Remove this line if you don't need a FASTA file
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-11-30 09:32:24

@jfy133 jfy133 marked this pull request as ready for review November 30, 2023 04:34
subworkflows/local/nonpareil.nf Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@jfy133
Copy link
Member Author

jfy133 commented Nov 30, 2023

@nf-core-bot fix linting

@jfy133 jfy133 requested a review from a team November 30, 2023 04:52
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Should nonpareil be included here since the process NONPAREIL_NONPAREIL was included? If that's the case, also check other config files.
perform_shortread_redundancyestimation = false

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that should we also include perform_shortread_redundancyestimation = false in this config file.

Copy link
Member Author

@jfy133 jfy133 Nov 30, 2023

Choose a reason for hiding this comment

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

I decided it wasn't necesary as switching to falco does not modify the input files going into NONPAREIL in any way, so saving 🐻‍❄️ by running one less step

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense!

docs/output.md Outdated Show resolved Hide resolved
docs/output.md Outdated Show resolved Hide resolved
docs/output.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
subworkflows/local/nonpareil.nf Outdated Show resolved Hide resolved
Co-authored-by: Lili Andersson-Li <64467552+LilyAnderssonLee@users.noreply.github.com>
Copy link
Contributor

@LilyAnderssonLee LilyAnderssonLee left a comment

Choose a reason for hiding this comment

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

I ran the test locally, and the results look good.

@Midnighter
Copy link
Collaborator

Is it the linting that touched so many files? It's a bit confusing, but Lili already approved so that's also fine 🙂 .

@jfy133
Copy link
Member Author

jfy133 commented Nov 30, 2023

Is it the linting that touched so many files? It's a bit confusing, but Lili already approved so that's also fine 🙂 .

This is nf-test for you... I added 3 modules and updated one.

Each comes with the typical ~3 module files, but for each module an additional 3-4 for nf-test and the conda env file, so doubling each. And then I updated quite a few test configs 🤷‍♀️Screenshot_20231130-150252.png

@jfy133
Copy link
Member Author

jfy133 commented Nov 30, 2023

Thanks @LilyAnderssonLee !

@jfy133 jfy133 merged commit ffff2e6 into bouncy-basenji Nov 30, 2023
26 checks passed
@jfy133 jfy133 deleted the add-nonpareil branch November 30, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants