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

Pyrodigal #251

Merged
merged 6 commits into from
Apr 3, 2023
Merged

Pyrodigal #251

merged 6 commits into from
Apr 3, 2023

Conversation

jasmezz
Copy link
Collaborator

@jasmezz jasmezz commented Mar 28, 2023

Closes #213

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

@jasmezz jasmezz requested a review from louperelo March 28, 2023 14:27
@github-actions
Copy link

github-actions bot commented Mar 28, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit fac328d

+| ✅ 157 tests passed       |+
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-04-03 14:36:21

Copy link
Contributor

@louperelo louperelo 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 to me. Just check if the annotation default should be Prodigal or Pyrodigal now.
Did you test the run with Pyrodigal locally? The CI test don't cover it.

docs/usage.md Outdated
@@ -82,7 +82,7 @@ To prevent entire pipeline failures due to a single 'bad sample', nf-core/funcsc

When the annotation is run with Prokka, the resulting `.gbk` file passed to antiSMASH may produce the error `translation longer than location allows` and end the pipeline run. This Prokka bug has been reported before (see [discussion on GitHub](https://github.com/antismash/antismash/discussions/450)) and is not likely to be fixed soon.

> ⚠️ If antiSMASH is run for BGC detection, we recommend to **not** run Prokka for annotation but instead leave the default annotation tool Prodigal or switch to Bakta (for bacteria only!).
> ⚠️ If antiSMASH is run for BGC detection, we recommend to **not** run Prokka for annotation but instead leave the default annotation tool Pyrodigal or switch to Bakta (for bacteria only!).
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you set Pyrodigal as default now? I think the nextflow.config hasn't been changed, right?

Suggested change
> ⚠️ If antiSMASH is run for BGC detection, we recommend to **not** run Prokka for annotation but instead leave the default annotation tool Pyrodigal or switch to Bakta (for bacteria only!).
> ⚠️ If antiSMASH is run for BGC detection, we recommend to **not** run Prokka for annotation but instead leave the default annotation tool Prodigal or switch to Bakta (for bacteria only!). If no gbk file is needed, Pyrodigal should be chosen as a faster alternative to Prodigal. It produces exactly the same results as Prodigal with lower memory usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I would not go into details about Pyrodigal/Prodigal differences here, because this paragraph is about not using Prokka for Antismash. More detailed info about the annotation tool differences can be found at the parameters and output docs already.

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.

What's going on with all the meta.yml changes and the MultiQC version bump?

  • missing:
    • Citations
    • Nextflow schema update
    • modules.conf update (does it need any parameter options?)

@louperelo
Copy link
Contributor

Apparently some little layout things were changed in all modules to make them appear nicer on the website (@mashehu said).

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.

Still missing citations.md update but almost there now!

@jasmezz jasmezz requested a review from jfy133 March 29, 2023 11:34
@jasmezz jasmezz merged commit 4b5f36d into dev Apr 3, 2023
@jasmezz jasmezz deleted the pyrodigal branch April 3, 2023 14:34
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.

3 participants