fix: bug with -O being used for gap opening penalty and samtools output directory #3
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
|
Clone of the PR snakemake/snakemake-wrappers#1450 |
![[development] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/334508?s=40&u=8e7ea115878aacba56242d7ac87807ea98eac709&v=4){kind=avatar}
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
1 similar comment
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new parameter, Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
| @@ -15,6 +15,7 @@ | |||
| log = snakemake.log_fmt_shell(stdout=False, stderr=True) | |||
| sort = snakemake.params.get("sorting", "none") | |||
| sort_extra = snakemake.params.get("sort_extra", "") | |||
| gap_opening = snakemake.params.get("gap_opening", "") | |||
There was a problem hiding this comment.
The new gap_opening parameter is retrieved without a default value. Consider providing a default value to ensure consistent behavior when the parameter is not explicitly set in the Snakemake rule. For example, you could modify the line to: gap_opening = snakemake.params.get("gap_opening", "10,3") (using minimap2's default values for gap open and extend penalties).
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
|
I have reviewed your code and found 3 potential issues. |
![development-korbit-ai-mentor[bot]](https://avatars.githubusercontent.com/in/334508?s=60&v=4){kind=small}
There was a problem hiding this comment.
The -O {gap_opening} option is currently being added to the minimap2 command unconditionally. This could potentially cause errors if the gap_opening parameter is empty or not applicable in certain scenarios. Consider adding a condition to only include this option when gap_opening is not empty. For example, you could modify the shell command to include this option conditionally:
shell(
"(minimap2"
" -t {snakemake.threads}"
" {extra} "
" {'-O ' + gap_opening if gap_opening else ''}"
" {snakemake.input.target}"
" {snakemake.input.query}"
" {pipe_cmd}"
" > {snakemake.output[0]}"
") {log}"
)This change will ensure that the -O option is only included when a gap opening penalty is actually specified.
Chat with Korbit by mentioning @development-korbit-ai-mentor, and give a 👍 or 👎 to help Korbit improve your reviews.
There was a problem hiding this comment.
Consider adding error handling for the shell command execution. Wrap the shell() function call in a try-except block to catch and handle any exceptions that may occur during the execution of minimap2 and samtools commands. This will ensure that errors are logged and the script exits gracefully in case of failures.
Chat with Korbit by mentioning @development-korbit-ai-mentor, and give a 👍 or 👎 to help Korbit improve your reviews.
There was a problem hiding this comment.
The newly added gap_opening parameter is a good addition to allow more flexibility in the alignment process. However, it's currently not being validated or handled properly. Consider adding input validation for this parameter and handling cases where it might be empty or invalid. For example, you could use a default value if it's not provided or raise an error if an invalid value is given. Also, it would be helpful to update the documentation to explain the usage and expected format of this new parameter.
Chat with Korbit by mentioning @development-korbit-ai-mentor, and give a 👍 or 👎 to help Korbit improve your reviews.
|
System configuration at time of review: |
|
@coderabbitai full review |
Actions performedFull review triggered. |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bio/minimap2/aligner/wrapper.py (2 hunks)
Additional context used
Ruff
bio/minimap2/aligner/wrapper.py
18-18: Undefined name
snakemake(F821)
Additional comments not posted (2)
bio/minimap2/aligner/wrapper.py (2)
18-18: Consider providing a default value forgap_opening.The
gap_openingparameter is retrieved without a default value. Consider providing a default value to ensure consistent behavior when the parameter is not explicitly set in the Snakemake rule. For example, you could modify the line to:gap_opening = snakemake.params.get("gap_opening", "10,3")(using minimap2's default values for gap open and extend penalties).
Tools
Ruff
18-18: Undefined name
snakemake(F821)
49-49: Include the-Ooption conditionally.The
-O {gap_opening}option is currently being added to the minimap2 command unconditionally. This could potentially cause errors if thegap_openingparameter is empty or not applicable in certain scenarios. Consider adding a condition to only include this option whengap_openingis not empty. For example, you could modify the shell command to include this option conditionally:shell( "(minimap2" " -t {snakemake.threads}" " {extra} " " {'-O ' + gap_opening if gap_opening else ''}" " {snakemake.input.target}" " {snakemake.input.query}" " {pipe_cmd}" " > {snakemake.output[0]}" ") {log}" )This change will ensure that the
-Ooption is only included when a gap opening penalty is actually specified.
|
/review |
PR Reviewer Guide 🔍
|
|
/review |
Description
Gap penalty of minimap2 is done via "-O" option and the very same option also exists for samtools, but to indicate output directory. Using "-O" in the "extra" leads to the following error because of using snakemake utils for samtools (https://github.com/snakemake/snakemake-wrapper-utils/blob/f9cba17cb380dc7e6729a845ae37f26972c3d1dd/snakemake_wrapper_utils/samtools.py#LL76C23-L76C23):
The fastest and not over-engineered solution to me was to add the gap_opening variable to params, but I feel that it's also not very elegant especially when it comes to modifying the other penalty scores, but they can still be set in the extra.
QC
For all wrappers added by this PR,
input:andoutput:file paths in the resulting rule can be changed arbitrarily,threads: xstatement withxbeing a reasonable default,map_readsfor a step that maps reads),environment.yamlspecifications follow the respective best practices,input:oroutput:),Snakefiles and their entries are explained via comments (input:/output:/params:etc.),stderrand/orstdoutare logged correctly (log:), depending on the wrapped tool,tempfile.gettempdir()points to (see here; this also means that using any Pythontempfiledefault behavior works),meta.yamlcontains a link to the documentation of the respective tool or command,Snakefiles pass the linting (snakemake --lint),Snakefiles are formatted with snakefmt,Description by Korbit AI
What change is being made?
Fix the bug by adding a parameter for gap opening penalty (
-O) in the minimap2 aligner wrapper and correct the samtools output directory handling.Why are these changes being made?
The gap opening penalty parameter was previously missing, causing incorrect alignment results. Additionally, the samtools output directory handling needed correction to ensure proper file output locations. These changes ensure accurate alignment and correct file management.
Summary by CodeRabbit
gap_openingparameter to enhance the configurability of the minimap2 alignment process, allowing users to specify gap opening penalties during alignment.gap_openingparameter for seamless integration with the minimap2 tool.