feat: add toulligqc wrapper #3171
Conversation
|
Warning Rate limit exceeded@fgvieira has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 55 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces several new files and configurations for the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
bio/toulligqc/wrapper.py (1)
9-12: Consider usingpathlib.Pathfor handling file paths and extensions.To make the code more robust and readable, consider using
pathlib.Pathto handle file paths and extensions. Here's an example:from pathlib import Path input_path = Path(snakemake.input[0]) if input_path.suffix in [".fastq", ".fastq.gz", ".fasta"]: input_flag = "--fastq " elif input_path.suffix == ".bam": input_flag = "--bam " else: input_flag = "--sequencing-summary-source "This approach has several benefits:
- It makes the code more readable by using descriptive variable names (
input_path) and methods (.suffix).- It eliminates the need for multiple
ifstatements and makes the code more concise.- It handles file extensions in a case-sensitive manner, which is more reliable.
Tools
Ruff
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
11-11: Undefined name
snakemake(F821)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- bio/toulligqc/environment.yaml (1 hunks)
- bio/toulligqc/meta.yaml (1 hunks)
- bio/toulligqc/test/Snakefile (1 hunks)
- bio/toulligqc/wrapper.py (1 hunks)
- test.py (1 hunks)
Files skipped from review due to trivial changes (1)
- bio/toulligqc/environment.yaml
Additional context used
Path-based instructions (2)
bio/toulligqc/wrapper.py (2)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Pattern
**/wrapper.py: Do not complain about use of undefined variable calledsnakemake.test.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
yamllint
bio/toulligqc/meta.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Ruff
bio/toulligqc/wrapper.py
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
11-11: Undefined name
snakemake(F821)
13-13: Undefined name
snakemake(F821)
14-14: Undefined name
snakemake(F821)
Additional comments not posted (4)
bio/toulligqc/test/Snakefile (1)
1-11: LGTM!The Snakemake rule for
toulligqcis correctly defined with the necessary directives:
input: Specifies the input file.output: Specifies the output file.log: Specifies the log file.params: Specifies optional additional parameters.wrapper: Specifies the wrapper to use.The code changes are approved.
bio/toulligqc/meta.yaml (1)
1-12: LGTM!The metadata file for
toulligqcis correctly defined with the necessary fields:
name: Specifies the name of the project.description: Provides a brief description of the project.url: Specifies the URL to the project's documentation.authors: Lists the authors of the project.input: Describes the input data.output: Describes the output data.params: Describes the optional parameters.The code changes are approved.
Tools
yamllint
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
bio/toulligqc/wrapper.py (1)
1-19: LGTM!The Python script for the
toulligqcwrapper is correctly implemented:
- It handles different input file types (
.fastq,.fastq.gz,.fasta,.bam) and sets the appropriate input flag.- It retrieves the optional
extraparameters fromsnakemake.params.- It constructs the
toulligqccommand with the necessary arguments and runs it usingsnakemake.shell.The code changes are approved.
Tools
Ruff
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
9-9: Undefined name
snakemake(F821)
11-11: Undefined name
snakemake(F821)
13-13: Undefined name
snakemake(F821)
14-14: Undefined name
snakemake(F821)
test.py (1)
6917-6927: LGTM!The new
test_toulligqcfunction looks good. It follows the established pattern for test functions in this file.
Co-authored-by: Filipe G. Vieira <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bio/toulligqc/wrapper.py (1 hunks)
Additional context used
Path-based instructions (1)
bio/toulligqc/wrapper.py (2)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Pattern
**/wrapper.py: Do not complain about use of undefined variable calledsnakemake.
Ruff
bio/toulligqc/wrapper.py
9-9: Undefined name
snakemake(F821)
10-10: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
21-21: Undefined name
snakemake(F821)
22-22: Undefined name
snakemake(F821)
Additional comments not posted (5)
bio/toulligqc/wrapper.py (5)
1-4: LGTM!The metadata variables are properly defined and contain appropriate information.
6-7: LGTM!The imported modules are necessary and properly used in the script.
9-19: Great job addressing the past review comments!The code segment correctly determines the input flag based on the file type and handles compressed files as suggested in the previous review. The supported file extensions have also been expanded to include
.fasand.fq.Tools
Ruff
9-9: Undefined name
snakemake(F821)
10-10: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
21-22: LGTM!The code segment correctly retrieves extra parameters and formats the logging output using the
snakemakeobject. The static analysis hint aboutsnakemakebeing an undefined name can be ignored because it is a valid object in the context of a Snakemake wrapper script.Tools
Ruff
21-21: Undefined name
snakemake(F821)
22-22: Undefined name
snakemake(F821)
26-29: LGTM!The empty lines at the end of the file are acceptable and do not affect the functionality of the script.
|
@fgvieira Thank you for all the edits! |
|
Sure. 😄 |
Yes of course, I'll work on that next week! |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
bio/toulligqc/test/Snakefile (2)
8-9: Consider adding documentation for extra parameters.The "extra" parameter in the params section provides flexibility, but there's no guidance on what kinds of additional parameters are allowed or useful.
Consider adding a comment or docstring explaining the possible extra parameters:
rule toulligqc: params: - extra="", # optional additional parameters as string + extra="", # optional additional parameters as string + # Possible extra parameters include: + # --min_pass_qual: Minimum quality score for a read to be considered as pass + # --fast5_dir: Directory containing FAST5 files + # For more options, see: https://toulligqc.readthedocs.io/en/latest/usage.htmlThis will help users understand how to utilize the extra parameter effectively.
1-11: Overall assessment: Good structure with room for improvement.The Snakefile for the toulligqc wrapper is well-structured and follows Snakemake conventions. However, there are several areas where it could be improved:
- Input and output paths could be parameterized for better flexibility in testing.
- The log file path could be more specific to avoid potential conflicts.
- Documentation for the extra parameters could be expanded.
- The wrapper version should be pinned to a specific version or commit hash for reproducibility.
Implementing these suggestions will enhance the flexibility, usability, and reproducibility of this Snakefile.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- bio/toulligqc/meta.yaml (1 hunks)
- bio/toulligqc/test/Snakefile (1 hunks)
- bio/toulligqc/wrapper.py (1 hunks)
- test.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- bio/toulligqc/meta.yaml
Additional context used
Path-based instructions (2)
bio/toulligqc/wrapper.py (2)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Pattern
**/wrapper.py: Do not complain about use of undefined variable calledsnakemake.test.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Ruff
bio/toulligqc/wrapper.py
9-9: Undefined name
snakemake(F821)
10-10: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
23-23: Undefined name
snakemake(F821)
Additional comments not posted (7)
bio/toulligqc/test/Snakefile (1)
1-1: LGTM: Rule declaration is clear and follows conventions.The rule name
toulligqcis descriptive and follows the snake_case naming convention, which is consistent with Snakemake best practices.bio/toulligqc/wrapper.py (5)
1-4: LGTM: Metadata section is complete and well-formatted.The metadata section provides all necessary information including author, copyright, email, and license. This follows good practices for code documentation.
6-7: LGTM: Imports are appropriate for a Snakemake wrapper.The imports of
shellfromsnakemake.shellandPathfrompathlibare correct and necessary for the functionality of this wrapper.
9-14: LGTM: Variable initialization and .gz handling are well-implemented.The initialization of
extraandlogvariables follows Snakemake wrapper conventions. The handling of.gzfile extensions is a good practice for flexibility.Note: The static analysis tool flags
snakemakeas an undefined variable, but this is expected in Snakemake wrappers as it's implicitly provided by the Snakemake framework.Tools
Ruff
9-9: Undefined name
snakemake(F821)
10-10: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
16-25: LGTM: Main execution block addresses previous concerns and implements correct logic.The code effectively determines the appropriate input flag based on file extensions, including
.fastq,.fq, and.bam. The output directory is correctly inferred from the first output file, addressing a previous concern. The shell command is well-constructed, including all necessary parameters and the--forceflag as suggested in a past review.This implementation successfully addresses the feedback from previous reviews and provides a flexible and robust execution of the toulligqc tool.
Tools
Ruff
23-23: Undefined name
snakemake(F821)
1-25: Excellent implementation of the toulligqc Snakemake wrapper.This wrapper successfully implements the toulligqc tool within the Snakemake framework. Key points:
- Proper metadata and imports are in place.
- Variable initialization follows Snakemake conventions.
- The code handles various input file types (FASTQ, FQ, BAM) and compressed (.gz) files.
- Output directory is correctly inferred from the output files.
- The shell command is well-constructed with all necessary parameters.
The implementation addresses all concerns raised in previous reviews and provides a flexible, robust solution for integrating toulligqc into Snakemake workflows.
Tools
Ruff
9-9: Undefined name
snakemake(F821)
10-10: Undefined name
snakemake(F821)
12-12: Undefined name
snakemake(F821)
23-23: Undefined name
snakemake(F821)
test.py (1)
6943-6955: LGTM! The new test function for ToulligQC is correctly implemented.The
test_toulligqc()function follows the same pattern as other test functions in this file:
- It uses the
@skip_if_not_modifieddecorator.- It calls the
run()function with appropriate arguments.- The Snakemake command is correctly formatted with the expected output file and necessary flags.
The function tests the ToulligQC wrapper by running Snakemake to generate the
report.htmlfile in thetoulligqc_sequencing_summarydirectory.
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
bio/toulligqc/test/demodata/test.fastq.gzis excluded by!**/*.gz
Files selected for processing (1)
- bio/toulligqc/test/Snakefile (1 hunks)
Additional comments not posted (1)
bio/toulligqc/test/Snakefile (1)
6-7: Use a more specific log file pathThe current log file path might lead to conflicts or overwriting if multiple runs are performed. Consider using a more specific log file path:
log: - "merged.log", + "logs/toulligqc_sequencing_summary.log",This change will help prevent log file conflicts and make it easier to associate logs with specific runs.
Likely invalid or redundant comment.
|
@fgvieira Hello! I have submitted the test bam and fast files, is there anything else needed for this PR to be approved? :) |
|
you need to add the tests to the |
|
Thanks for your contribution. Great work! 😄 |
🤖 I have created a release \*beep\* \*boop\* --- ## [4.6.0](https://www.github.com/snakemake/snakemake-wrappers/compare/v4.5.0...v4.6.0) (2024-09-25) ### Features * add toulligqc wrapper ([#3171](https://www.github.com/snakemake/snakemake-wrappers/issues/3171)) ([874c84e](https://www.github.com/snakemake/snakemake-wrappers/commit/874c84e8488a9f04e3c1805553aaa03c580e732c)) ### Performance Improvements * bump datavzrd wrapper ([#3271](https://www.github.com/snakemake/snakemake-wrappers/issues/3271)) ([a08e006](https://www.github.com/snakemake/snakemake-wrappers/commit/a08e006e8da99c9d06032170684dc943d13f4665)) * bump vembrane wrappers to latest version ([#3269](https://www.github.com/snakemake/snakemake-wrappers/issues/3269)) ([2a36586](https://www.github.com/snakemake/snakemake-wrappers/commit/2a36586024da42cb538f53c958c6ffc6207cb3ac)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Added wrapper for ToulligQC which is a quality control tool for nanopore data.
QC
snakemake-wrappers.While the contributions guidelines are more extensive, please particularly ensure that:
test.pywas updated to call any added or updated example rules in aSnakefileinput:andoutput:file paths in the rules can be chosen arbitrarilyinput:oroutput:)tempfile.gettempdir()points tometa.yamlcontains a link to the documentation of the respective tool or command underurl:Summary by CodeRabbit
New Features
toulligqcproject, simplifying setup for users.toulligqctool, including descriptions and input/output specifications, enhancing user understanding.toulligqccommand-line tool, improving flexibility in handling different input file types.Tests
toulligqcprocess to ensure functionality is covered.