fix: bwa-memx-index rule uses correct suffixarray path; fixes #1344 #3303
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
bio/bwa-memx/index/meta.yaml (2)
10-12: LGTM with a minor suggestion: Clear parameter specificationsThe params section clearly defines the 'bwa' and 'num_models' parameters. The descriptions are informative and include necessary details like available options and default values.
Consider adding a note that the 'num_models' parameter is only applicable when 'bwa' is set to "bwa-meme". This could prevent confusion for users who might try to use it with other BWA variants.
🧰 Tools
🪛 yamllint
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
12-12: Add a newline at the end of the fileTo adhere to common conventions and prevent potential issues with some tools, please add a newline character at the end of the file.
You can do this by adding an empty line at the end of the file.
🧰 Tools
🪛 yamllint
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bio/bwa-meme/index/wrapper.py (1 hunks)
- bio/bwa-memx/index/meta.yaml (1 hunks)
- bio/bwa-memx/index/wrapper.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
bio/bwa-meme/index/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.bio/bwa-memx/index/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.
🪛 yamllint
bio/bwa-memx/index/meta.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
bio/bwa-memx/index/meta.yaml (3)
5-5: LGTM: Author additionAdding Patrik Smeds to the authors list is appropriate for giving credit to contributors.
6-7: LGTM: Input specificationThe input section clearly specifies the requirement for a fasta file, which is correct for BWA index creation.
8-9: LGTM: Comprehensive output descriptionThe output section provides a clear and detailed description of the expected BWA index files. The note about the common prefix and the example of including an output directory path are particularly helpful for users.
bio/bwa-meme/index/wrapper.py (2)
48-48: LGTM! This change improves flexibility and consistency.The modification to the
suffixarrayvariable construction addresses the issue mentioned in the PR objectives. By using theprefixvariable in an f-string, it allows for an arbitrary output directory while ensuring that thesuffixarrayfile shares the same prefix as other output files. This change is consistent with the error checking done earlier in the script and improves the overall flexibility of the wrapper.
Line range hint
1-62: Verify corresponding changes in related fileThe change made in this file addresses the issue described in the PR objectives. However, to ensure consistency across the project, we should verify that similar changes have been made in the
bio/bwa-memx/index/wrapper.pyfile, as mentioned in the AI summary.✅ Verification successful
Changes Verified in Related File
The
bio/bwa-memx/index/wrapper.pyfile contains the necessary updates consistent with the modifications inbio/bwa-meme/index/wrapper.py.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if similar changes have been made in the bwa-memx wrapper # Test: Look for similar suffixarray construction in bwa-memx wrapper rg -A 3 'suffixarray =' bio/bwa-memx/index/wrapper.pyLength of output: 261
🧰 Tools
🪛 Ruff
51-51: Undefined name
snakemake(F821)
bio/bwa-memx/index/wrapper.py (1)
77-77: Update tosuffixarrayvariable enhances naming consistencyThe construction of
suffixarrayusing theprefixvariable ensures consistent file naming for the output files. This change aligns with similar modifications in related wrapper scripts and enhances the robustness of the file handling.
fgvieira
changed the title
🤖 I have created a release \*beep\* \*boop\* --- ### [4.7.3](https://www.github.com/snakemake/snakemake-wrappers/compare/v4.7.2...v4.7.3) (2024-10-23) ### Bug Fixes * bwa-memx-index rule uses correct suffixarray path; fixes [#1334](https://www.github.com/snakemake/snakemake-wrappers/issues/1334) ([#3303](https://www.github.com/snakemake/snakemake-wrappers/issues/3303)) ([2256246](https://www.github.com/snakemake/snakemake-wrappers/commit/2256246fc6fab81444253119a06c273bd86ee325)) * Hisat2 index improvements ([#3305](https://www.github.com/snakemake/snakemake-wrappers/issues/3305)) ([7647997](https://www.github.com/snakemake/snakemake-wrappers/commit/76479978e33e2b5a85100f5c90457ad1eeb75b95)) --- 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>
The wrapper previously did not allow arbitrary output directory for the files. This was due to how the file defined in the
suffixarrayvariable was being fetched from the input, rather than from the output prefix. I have:meta.yml)pytest test.py -v -k test_bwa_memx_indexwhich ran successfullyIf someone else could revise and test that would be great.
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
Bug Fixes
Documentation
meta.yamlfile for clarity and accuracy.