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

Add new module for gene annotation: Bakta #95

Merged
merged 12 commits into from
Oct 20, 2023
Merged

Add new module for gene annotation: Bakta #95

merged 12 commits into from
Oct 20, 2023

Conversation

Daniel-VM
Copy link
Contributor

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/bacass 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).
  • 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).

PR description

Subworkflow added to perform gene annotation with Bakta .
Closes #94

MultiQC has been updated to v1.17. This version has a multiqc module for Bakta.

Important notes

  • No nf-core/bacass tests (conf/test*.conf) has been added for Bakta because downloading the db-light.gz (1.3G) in Zenodo is time consuming and slowdowns the testing process. Although, we can add bakta as a new conf/test*.config file. For this reason, instead of substituting PROKKA (Removal of Prokka in favor of Bakta #94), Bakta has been added as an additional tool for gene annotation together with prokka and dfast.

  • Bakta v1.8.2 fails due to dependency issue with the amrfinder-plus database. This issue has been overcomed by downloading the bakta database with bakta v1.8.1 and perforiming gene annotation wit bakta v1.8.2. Detailed info --> bakta#issue211 and bakta#issue241.

@github-actions
Copy link

github-actions bot commented Oct 19, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 3c79c45

+| ✅ 158 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in README.md: Describe the minimum required steps to execute the pipeline, e.g. how to prepare samplesheets.
  • pipeline_todos - TODO string in README.md: update the following command to include all required parameters for a minimal example
  • 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
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in WorkflowBacass.groovy: Optionally add in-text citation tools to this list.

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-10-20 09:13:33

d4straub
d4straub reviewed Oct 19, 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.

Looks good, but I found a few points.

CHANGELOG.md Outdated Show resolved Hide resolved
modules/nf-core/bakta/bakta/bakta-bakta.diff Outdated Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
@Daniel-VM
Copy link
Contributor Author

Thanks for the review ;)!

d4straub
d4straub approved these changes Oct 20, 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.

Great, looks good to me!

About the changelog: Looks better now, but I think there arent multiple headlines needed for 2.1.0dev but all points can be summarized under one headline. As example see here where one headline summarizes the change between releases (which are typically several months away, many PRs).

I would really recommend soon a release, after a release your work doesnt need to stop, but the reviewing will be easier and another release can always be done in quick succession. Here is a detailed guide if you are wondering how that works: https://nf-co.re/docs/contributing/release_checklist (if you are not up to it I can also do it). If you have only a few more additions before a release, thats of course also fine.

@Daniel-VM Daniel-VM closed this Oct 20, 2023
@Daniel-VM Daniel-VM reopened this Oct 20, 2023
@Daniel-VM
Copy link
Contributor Author

Sorry I accidentally close it.

@d4straub
Copy link
Collaborator

A quick question here. The date in the headline should point to the version release date or the last update date?

Here is the changelog of a dev branch (release was yesterday/today so no additions yet), i.e. the date is the date of the release and I use no date for the dev branch. You could of course add something like "last changed " or such if you like.

@Daniel-VM
Copy link
Contributor Author

Daniel-VM commented Oct 20, 2023 •
edited
Loading

Regarding your last comments.
I am going to collapse all entries for 2.1.0dev in a single headline.
A quick question . Should the date in the headline correspond to the version release date or the last update date (lets say today)?. In other words, do I have to update the date as well or keep the release date of thee version? [answered in previous comment]

Great! I could try the release. Actually, I am planning to move forward to the assembly step (new strategies and modules). This will might include major changes, so I think that we can proceed with the release. wdyt?

@Daniel-VM Daniel-VM merged commit 6bd6045 into nf-core:dev Oct 20, 2023
@Daniel-VM Daniel-VM mentioned this pull request Oct 20, 2023
Closed
@d4straub
Copy link
Collaborator

I think releases are always good to have. Smaller but more frequent releases are better than huge but infrequent releases. But I myself do not follow that idea typically ;)

@Daniel-VM Daniel-VM mentioned this pull request Oct 23, 2023
Merged
10 tasks
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.
2 participants
@Daniel-VM @d4straub