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

RC 3.16.0 #1395

Merged
merged 121 commits into from
Oct 2, 2024
Merged

RC 3.16.0 #1395

merged 121 commits into from
Oct 2, 2024

Conversation

maxulysse
Copy link
Member

@maxulysse maxulysse commented Sep 30, 2024

Release Candidate 3.16.0 aka Fire Ferret

Merge only once #1394 is merged

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/rnaseq 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>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

Copy link

github-actions bot commented Sep 30, 2024

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 00a0857

+| ✅ 173 tests passed       |+
#| ❔   9 tests were ignored |#
!| ❗   8 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml
  • nextflow_config - Config manifest.version should end in dev: 3.16.0
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • 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 ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-10-02 12:25:51

@maxulysse maxulysse changed the title RC 3.15.2 RC 3.16.0 Oct 1, 2024
@MatthiasZepper
Copy link
Member

What do we do with #1369 ?

@siddharthab made such an awesome effort, and it is essentially done - the tool is just horribly slow on transcriptomic alignments, so we are since weeks waiting that the tool's author will merge a tiny PR that fixes that. I am seriously considering forking the repo and merge it there...because I so badly want that tool in the pipeline.

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Only thing I would say I'm unhappy with is the extremely minimal description of the input file for --kraken2_db.

A kraken2 database is not the same as a bracken database (bracken requires multiple extra files alongside the normal kraken2 database files), and the Ben Langmead things I guess is being assumed is the ones most people use are not actually really the gold standard (there aren't any).

I would suggest improving the description of what gets supplied to that paramneter (a tar.gz) and also in usage explain further what is accepted and when depending on if you're supplying kraken vs bracken parameters.

But otherwise everything else is self-explanatory (thus the approval)

| `Bracken` | ----------- | 2.9 |

> **NB:** Dependency has been **updated** if both old and new version information is present.
>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>

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 like the line breaks here

> **NB:** Dependency has been **updated** if both old and new version information is present.
>
> **NB:** Dependency has been **added** if just the new version information is present.
>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>

},
"kraken_db": {
"type": "string",
"description": "Database when using Kraken2/Bracken for contaminant screening.",
Copy link
Member

Choose a reason for hiding this comment

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

In what form? Should this be a directory or a tar.gz file?

Copy link
Member

Choose a reason for hiding this comment

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

OR both? The Ben Langmead versions are tar.gz for example

Copy link
Member Author

Choose a reason for hiding this comment

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

@egreenberg7 any idea about that?

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'll make sure to include the answer to that when we make tests for this new feature in the next release

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a tar.gz file since the nf-core Kraken2 and Bracken modules use the --gzip-compressed option. It probably should be clarified that kraken2 and bracken databases are distinct, but they are often zipped together (as in the pre-built indices). A separate --bracken_db option potentially could be added.

While the pre-built files are not gold standard, they provide a quick/convenient option for people who want to run contamination detection without getting into the weeds of the tools. I'll note that I did not offer the ability to make a standard database in an indexing run of the pipeline because it is so much slower than just downloading pre-built indices (and rather computationally expensive).

In any case, @jfy133 has much more experience than me with metagenomics tools, so I would defer to any changes he recommends to either the Usage tab or the input

subworkflows/local/utils_nfcore_rnaseq_pipeline/main.nf Outdated Show resolved Hide resolved
subworkflows/local/utils_nfcore_rnaseq_pipeline/main.nf Outdated Show resolved Hide resolved
//Check that Kraken/Bracken parameters are not provided when Kraken2 is not being used
} else {
if (!params.bracken_precision.equals('S')) {
brackenPrecisionWithoutKrakenDBWarn()
Copy link
Member

Choose a reason for hiding this comment

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

Is a function here really necessary? You only use it once...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot, that'll be looked after in some slight refactoring, but out of scope for this release

tests/default.nf.test Show resolved Hide resolved
workflows/rnaseq/main.nf Show resolved Hide resolved
@maxulysse
Copy link
Member Author

What do we do with #1369 ?

@siddharthab made such an awesome effort, and it is essentially done - the tool is just horribly slow on transcriptomic alignments, so we are since weeks waiting that the tool's author will merge a tiny PR that fixes that. I am seriously considering forking the repo and merge it there...because I so badly want that tool in the pipeline.

This was intended to be just a patch release, Kraken2/Bracken was already in so we release as 3.16.0, but it was really meant to be 3.15.2

#1369 will be included in the next release

Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@MatthiasZepper
Copy link
Member

What do we do with #1369 ?
@siddharthab made such an awesome effort, and it is essentially done - the tool is just horribly slow on transcriptomic alignments, so we are since weeks waiting that the tool's author will merge a tiny PR that fixes that. I am seriously considering forking the repo and merge it there...because I so badly want that tool in the pipeline.

This was intended to be just a patch release, Kraken2/Bracken was already in so we release as 3.16.0, but it was really meant to be 3.15.2

#1369 will be included in the next release

Fine with me.

BTW: You are not very patriotic, are you? Why would you chose "Fire" as an element, if you could have chosen Francium and honor your home country 😉

@maxulysse
Copy link
Member Author

What do we do with #1369 ?
@siddharthab made such an awesome effort, and it is essentially done - the tool is just horribly slow on transcriptomic alignments, so we are since weeks waiting that the tool's author will merge a tiny PR that fixes that. I am seriously considering forking the repo and merge it there...because I so badly want that tool in the pipeline.

This was intended to be just a patch release, Kraken2/Bracken was already in so we release as 3.16.0, but it was really meant to be 3.15.2
#1369 will be included in the next release

Fine with me.

BTW: You are not very patriotic, are you? Why would you chose "Fire" as an element, if you could have chosen Francium and honor your home country 😉

https://avatar.fandom.com/wiki/Fire_ferret

Copy link
Contributor

Choose a reason for hiding this comment

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

Weeee

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, so much ignored!

@maxulysse maxulysse merged commit 33df0c0 into master Oct 2, 2024
48 checks passed
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.

9 participants