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

new module Bakta #1959

Merged
merged 13 commits into from
Aug 31, 2023
Merged

new module Bakta #1959

merged 13 commits into from
Aug 31, 2023

Conversation

louperelo
Copy link
Contributor

@louperelo louperelo commented Jul 25, 2023

Bakta is a tool for rapid and standardized annotation of bacterial genomes, MAGs and plasmids. We are using Bakta as one of the annotation tool options in nf-core/funcscan and would like to include its output in the pipelines MultiQC report.

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated
  • There is example tool output for tools in the https://github.com/ewels/MultiQC_TestData repository: #258
  • Code is tested and works locally (including with --lint flag)
  • docs/README.md is updated with link to below
  • docs/modulename.md is created
  • Everything that can be represented with a plot instead of a table is a plot
  • Report sections have a description and help text (with self.add_section)
  • There aren't any huge tables with > 6 columns (explain reasoning if so)
  • Each table column has a different colour scale to its neighbour, which relates to the data (eg. if high numbers are bad, they're red)
  • Module does not do any significant computational work

louperelo and others added 2 commits August 25, 2023 11:41
Co-authored-by: Vlad Savelyev <vladislav.sav@gmail.com>
@louperelo
Copy link
Contributor Author

Thank you for the review @vladsavelyev! Please check if I resolved the merge conflict for the CHANGELOG.md and README.md correctly.

"title": "# contigs",
"description": "Number of contigs",
"min": 0,
"format": "{:i}%",
Copy link
Member

Choose a reason for hiding this comment

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

But it's not a percentage? Also, I don't think {:i} is a recognised format string. At least it doesn't seem to work, as numbers are displayed in the table as string (you can tell that by the lack of thousand groups separators!). I'll change them to "{:,d}" if that's okay.

@vladsavelyev vladsavelyev self-requested a review August 25, 2023 15:26
Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

@louperelo - thanks for responding to the review quickly! Last time I didn't complete it, just left a few quick suggestions 😅 Now it's done. I mostly merged my suggestions directly into the branch, hope it's okay. Here is what I changed:

  • fixed the format strings to "format": "{:,d}"
  • table colors - I tried to make them match similar metrics in the QUAST module, plus added a shared_key: gene_count for the CDS count to make the number scale comparable with QUAST
  • got rid of spaces in the metric keys - it's not a problem for MultiQC, but theoretically might be some downstream tools that might use MultiQC output
  • iterating over f["contents_lines"] to avoid extra splitting
  • in search patterns, removed num_lines: 26 to make it more future-proof

@vladsavelyev vladsavelyev added the awaits-review Awaiting final review and merge. label Aug 25, 2023
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ewels ewels merged commit 38e8d2f into MultiQC:master Aug 31, 2023
4 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits-review Awaiting final review and merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants