revisit demultiplexing, sv cli commands #191
Conversation
|
Warning Rate limit exceeded@pdimens has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 33 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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces a new conda environment category named "demultiplex" along with its associated dependencies. It refactors the presentation of workflow information by adding a common Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Demultiplex Command
participant F as fetch_script (_misc)
participant W as workflow_info (_printing)
participant S as launch_snakemake
U->>D: Execute demultiplex command with parameters
D->>F: Fetch "demultiplex_gen1.py" script
F-->>D: Return script content
D->>W: Generate formatted workflow information
W-->>D: Return start_text
D->>S: Invoke launch_snakemake with start_text
S-->>D: Process workflow execution
Assessment against linked issues
Suggested labels
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: 4
🧹 Nitpick comments (3)
harpy/scripts/demultiplex_gen1.py (2)
52-52: Avoid calling.keys()explicitly for membershipThe code uses
.keys()unnecessarily. Iteration over a dictionary automatically iterates its keys, which is slightly more idiomatic.-for seq in bar_codes[code_letter].keys(): +for seq in bar_codes[code_letter]:🧰 Tools
🪛 Ruff (0.8.2)
52-52: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
66-66: Ambiguous variable nameIThe variable
Ican be confused withl(lowercase L) or1(one). Use a more descriptive name to improve readability.For example:
-def get_read_codes(I, codeL, codeR): +def get_read_codes(barcode_seq, codeL, codeR):🧰 Tools
🪛 Ruff (0.8.2)
66-66: Ambiguous variable name:
I(E741)
harpy/snakefiles/demultiplex_gen1.smk (1)
147-161: Improve input file path presentation in workflow summary.The input file paths could be formatted more consistently for better readability.
Apply this diff to improve formatting:
- inputs += f"\tread 1: {params.R1}\n" - inputs += f"\tread 2: {params.R2}\n" - inputs += f"\tindex 1: {params.I1}\n" - inputs += f"\tindex 2: {params.I2}" - inputs += f"Sample demultiplexing schema: {samplefile}" + inputs += f"\tRead 1: {params.R1}\n" + inputs += f"\tRead 2: {params.R2}\n" + inputs += f"\tIndex 1: {params.I1}\n" + inputs += f"\tIndex 2: {params.I2}\n" + inputs += f"\tSchema: {samplefile}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
harpy/_conda.py(1 hunks)harpy/_printing.py(1 hunks)harpy/align.py(4 hunks)harpy/assembly.py(2 hunks)harpy/bin/dmux_gen1.py(0 hunks)harpy/bin/haplotag_acbd.py(1 hunks)harpy/deconvolve.py(2 hunks)harpy/demultiplex.py(4 hunks)harpy/downsample.py(2 hunks)harpy/impute.py(2 hunks)harpy/metassembly.py(2 hunks)harpy/phase.py(2 hunks)harpy/preflight.py(3 hunks)harpy/qc.py(2 hunks)harpy/resume.py(2 hunks)harpy/scripts/demultiplex_gen1.py(1 hunks)harpy/simulate.py(10 hunks)harpy/snakefiles/demultiplex_gen1.smk(3 hunks)harpy/snp.py(3 hunks)harpy/sv.py(3 hunks)
💤 Files with no reviewable changes (1)
- harpy/bin/dmux_gen1.py
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/scripts/demultiplex_gen1.py
17-17: Undefined name parser
(F821)
52-52: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
66-66: Ambiguous variable name: I
(E741)
86-86: Undefined name snakemake
(F821)
88-88: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Undefined name snakemake
(F821)
94-94: Undefined name snakemake
(F821)
95-95: Undefined name snakemake
(F821)
96-96: Undefined name snakemake
(F821)
97-97: Undefined name snakemake
(F821)
110-110: Use a context manager for opening files
(SIM115)
111-111: Use a context manager for opening files
(SIM115)
121-121: Undefined name snakemake
(F821)
122-122: Undefined name snakemake
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (25)
harpy/scripts/demultiplex_gen1.py (1)
86-123: Undefinedsnakemakereferences and file handles in dictionaries
Frequent references to
snakemake(lines 86-103, 121-122) may raise aNameErrorin a plain Python context. In Snakemake scripts, this variable is globally injected, so this is valid only if run within Snakemake.Multiple file handles are stored in dictionaries (lines 110-111), and they are manually closed (lines 158-160). This is acceptable. However, if you want to leverage context managers for better safety and clarity, you could consider using
ExitStackfromcontextlib.Example for a safer pattern:
from contextlib import ExitStack import gzip import pysam with ExitStack() as stack: R1_output = { sample: stack.enter_context(gzip.open(f"{outdir}/{sample}.R1.fq.gz", "wb", compresslevel=6)) for sample in samples } # ... # further steps that ensure all opened files will be closed automaticallyPlease confirm if the code is run exclusively within Snakemake, where
snakemakeis a known global variable.✅ Verification successful
Snakemake Context Confirmation and File Handle Management
The code is intended to run exclusively within a Snakemake workflow—here, the global
snakemakevariable is injected automatically, so the references are valid in that context. The use of dictionaries to manage file handles, with manual closing, is acceptable though switching to a context manager likeExitStackcould further guarantee resource safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
🧰 Tools
🪛 Ruff (0.8.2)
86-86: Undefined name
snakemake(F821)
88-88: Undefined name
snakemake(F821)
89-89: Undefined name
snakemake(F821)
90-90: Undefined name
snakemake(F821)
91-91: Undefined name
snakemake(F821)
92-92: Undefined name
snakemake(F821)
93-93: Undefined name
snakemake(F821)
94-94: Undefined name
snakemake(F821)
95-95: Undefined name
snakemake(F821)
96-96: Undefined name
snakemake(F821)
97-97: Undefined name
snakemake(F821)
110-110: Use a context manager for opening files
(SIM115)
111-111: Use a context manager for opening files
(SIM115)
121-121: Undefined name
snakemake(F821)
122-122: Undefined name
snakemake(F821)
harpy/resume.py (1)
50-53: Clean and consistent approach to workflow infoUsing
workflow_infoin place of manually constructing tables is a neat refactoring. This keeps the code concise and readable.harpy/bin/haplotag_acbd.py (1)
30-30: Changed output file extension from.txtto.bcThe new naming convention
segment_{BC}.bcappears domain-specific, which is fine if it's well-documented. Ensure downstream consumers can handle.bcas a tab-delimited text.harpy/deconvolve.py (1)
86-90: LGTM!The implementation correctly uses the new workflow_info function to create a consistent workflow information display.
harpy/downsample.py (1)
96-101: LGTM!The implementation correctly uses the new workflow_info function to create a consistent workflow information display.
harpy/demultiplex.py (5)
11-12: LGTM!The new imports enhance modularity by separating script fetching and workflow info presentation concerns.
60-60: LGTM!The docstring update improves clarity by better describing the barcode segment's purpose.
75-76: LGTM!The addition of script fetching enhances modularity by ensuring the demultiplexing script is available in the workflow directory.
79-79: LGTM!The addition of the "demultiplex" environment to conda_envs ensures proper dependency management.
104-109: LGTM!The refactored workflow info presentation using
workflow_infofunction improves consistency and maintainability.harpy/metassembly.py (2)
12-12: LGTM!The addition of
workflow_infoimport aligns with the codebase-wide refactoring for consistent output formatting.
101-106: LGTM!The refactored workflow info presentation using
workflow_infofunction improves consistency and maintainability.harpy/snakefiles/demultiplex_gen1.smk (3)
25-30: LGTM!The improved error handling with specific ValueError catch enhances code robustness.
38-40: LGTM!The updated file paths using workflow directory improve organization and maintainability.
46-69: LGTM!The refactored demultiplex rule with explicit input structure and improved organization enhances clarity and maintainability.
harpy/impute.py (2)
14-14: LGTM!The addition of
workflow_infoimport aligns with the codebase-wide refactoring for consistent output formatting.
109-117: LGTM!The refactored workflow info presentation using
workflow_infofunction improves consistency and maintainability.harpy/phase.py (1)
111-119: LGTM! Clean refactoring of workflow information display.The refactoring maintains all the necessary information while improving code consistency. The conditional handling of genome information is well implemented.
harpy/qc.py (1)
117-124: LGTM! Well-structured workflow information display.The refactoring effectively presents all workflow parameters with clear yes/no indicators for the operational flags.
harpy/assembly.py (1)
131-136: LGTM! Effective refactoring of assembly workflow information.The implementation cleanly handles both auto and custom kmer length displays while maintaining consistent formatting.
harpy/preflight.py (2)
93-97: LGTM! Clean implementation of BAM preflight information display.The refactoring maintains consistency with other workflow displays while clearly presenting BAM-specific information.
155-159: LGTM! Clean implementation of FASTQ preflight information display.The refactoring maintains parallel structure with the BAM implementation while clearly presenting FASTQ-specific information.
harpy/snp.py (1)
14-14: LGTM! Clean refactoring of workflow information presentation.The changes improve code consistency by using the new
workflow_infofunction to standardize the presentation of workflow information across different commands.Also applies to: 145-151, 249-255
harpy/sv.py (1)
14-14: LGTM! Consistent refactoring of workflow information presentation.The changes align with the codebase-wide standardization of workflow information presentation using the
workflow_infofunction.Also applies to: 135-141, 238-245
harpy/simulate.py (1)
262-267: LGTM! Enhanced error handling for variant simulation.The added error handling prevents users from specifying both random and known variants, providing clear error messages. This improves the robustness of the command-line interface.
Also applies to: 374-375, 484-485, 584-585
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
harpy/scripts/demultiplex_gen1.py (2)
49-65: Improve code readability and maintainability.The function has several areas for improvement:
- Replace the magic number
999with a named constant.- Use more descriptive variable names.
Apply this diff to improve the code:
+MAX_DISTANCE = 999 # Maximum possible Levenshtein distance def get_min_dist(needle, code_letter): - minDist = 999 - nbFound = 0 - minSeq ="" + min_distance = MAX_DISTANCE + matches_found = 0 + min_sequence = "" for seq in bar_codes[code_letter]: d = distance(needle, seq) - if (d < minDist): - minDist = d - nbFound = 1 - minSeq= seq - elif (d == minDist): - nbFound += 1 - if (nbFound>1): + if d < min_distance: + min_distance = d + matches_found = 1 + min_sequence = seq + elif d == min_distance: + matches_found += 1 + if matches_found > 1: code_min_dist = f"{code_letter}00" else: - code_min_dist = bar_codes[code_letter][minSeq] + code_min_dist = bar_codes[code_letter][min_sequence] return code_min_dist🧰 Tools
🪛 Ruff (0.8.2)
53-53: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
67-85: Improve code readability and maintainability.The function has several areas for improvement:
- Use a more descriptive parameter name instead of
I.- Extract the hardcoded slice length into a named constant.
Apply this diff to improve the code:
-def get_read_codes(I, codeL, codeR): +BARCODE_LENGTH = 6 # Length of each barcode sequence +def get_read_codes(sequence, code_left, code_right): - left = I[0:6] # change depending on protocol - right = I[7:] + left = sequence[0:BARCODE_LENGTH] + right = sequence[BARCODE_LENGTH + 1:] # Skip the separator character status = "found" - if left in bar_codes[codeL]: - lc = bar_codes[codeL][left] + if left in bar_codes[code_left]: + left_code = bar_codes[code_left][left] else: - lc = get_min_dist(left, codeL) + left_code = get_min_dist(left, code_left) status = "corrected" - if right in bar_codes[codeR]: - rc = bar_codes[codeR][right] + if right in bar_codes[code_right]: + right_code = bar_codes[code_right][right] else: - rc = get_min_dist(right, codeR) + right_code = get_min_dist(right, code_right) status = "corrected" - if (lc == f"{codeL}00" or rc == f"{codeR}00"): + if (left_code == f"{code_left}00" or right_code == f"{code_right}00"): status = "unclear" - return rc, lc, status + return right_code, left_code, status🧰 Tools
🪛 Ruff (0.8.2)
67-67: Ambiguous variable name:
I(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
harpy/_conda.py(1 hunks)harpy/_printing.py(1 hunks)harpy/_validations.py(1 hunks)harpy/align.py(4 hunks)harpy/scripts/demultiplex_gen1.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- harpy/_conda.py
- harpy/align.py
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/_validations.py
276-276: Undefined name seg_id
(F821)
284-284: Undefined name file_path
(F821)
harpy/scripts/demultiplex_gen1.py
53-53: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
67-67: Ambiguous variable name: I
(E741)
87-87: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Undefined name snakemake
(F821)
94-94: Undefined name snakemake
(F821)
95-95: Undefined name snakemake
(F821)
96-96: Undefined name snakemake
(F821)
97-97: Undefined name snakemake
(F821)
98-98: Undefined name snakemake
(F821)
111-111: Use a context manager for opening files
(SIM115)
112-112: Use a context manager for opening files
(SIM115)
122-122: Undefined name snakemake
(F821)
123-123: Undefined name snakemake
(F821)
🔇 Additional comments (1)
harpy/_printing.py (1)
104-115:⚠️ Potential issueFix the conditional check and add type safety.
The function has several issues that need to be addressed:
- The conditional check
if i:is incorrect as it checks the entire argument tuple instead of checking if it's None.- No validation for tuple/list length which could lead to runtime errors.
Apply this diff to fix the issues:
def workflow_info(*arg: tuple[str, str | int | float]) -> Table: """ Accepts an unlimited number of length-2 lists or tuples and returns a rich.Table with the value of the first indices as the row names and the second indices as the values Use None instead of a list to ignore that entry (useful for conditionals). The second value will always be converted to a string. """ table = Table(show_header=False,pad_edge=False, show_edge=False, padding = (0,0), box=box.SIMPLE) table.add_column("detail", justify="left", style="light_steel_blue", no_wrap=True) table.add_column("value", justify="left") for i in arg: - if i: + if i is not None and len(i) == 2: table.add_row(i[0], str(i[1])) + elif i is not None: + raise ValueError("Each argument must be a tuple/list of length 2") return tableLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
harpy/scripts/demultiplex_gen1.py (1)
83-170:🧩 Analysis chain
Several improvements needed in the main execution block.
- File handling should use context managers for output files.
- The script relies on undefined
snakemakevariable.- Variable
Iis ambiguous.Apply these improvements:
+#!/usr/bin/env python3 + +import argparse +import sys +import pysam +from pathlib import Path +from Levenshtein import distance + +def parse_args(): + parser = argparse.ArgumentParser(description='Demultiplex haplotagged FASTQ files') + parser.add_argument('--r1', required=True, help='Read 1 FASTQ file') + parser.add_argument('--r2', required=True, help='Read 2 FASTQ file') + parser.add_argument('--i1', required=True, help='Index 1 FASTQ file') + parser.add_argument('--i2', required=True, help='Index 2 FASTQ file') + parser.add_argument('--schema', required=True, help='Demultiplexing schema file') + parser.add_argument('--outdir', required=True, help='Output directory') + parser.add_argument('--qxrx', action='store_true', help='Include QX:Z and RX:Z tags') + return parser.parse_args() -with open(snakemake.log[0], "w") as f: - sys.stderr = sys.stdout = f - outdir = snakemake.params.outdir - qxrx = snakemake.params.qxrx +def main(): + args = parse_args() + outdir = args.outdir + qxrx = args.qxrx # Create output files using context managers output_files = {} try: for sample in samples: output_files[sample] = { 'R1': open(f"{outdir}/{sample}.R1.fq", 'w'), 'R2': open(f"{outdir}/{sample}.R2.fq", 'w') } with ( - pysam.FastxFile(r1) as R1, - pysam.FastxFile(r2) as R2, - pysam.FastxFile(i1, persist=False) as I1, - pysam.FastxFile(i2, persist=False) as I2, + pysam.FastxFile(args.r1) as read1, + pysam.FastxFile(args.r2) as read2, + pysam.FastxFile(args.i1, persist=False) as index1, + pysam.FastxFile(args.i2, persist=False) as index2, ): # Process reads... finally: # Ensure all files are closed for sample_files in output_files.values(): for f in sample_files.values(): f.close() +if __name__ == '__main__': + main()Run this script to check for undefined variables:
🏁 Script executed:
#!/bin/bash # Description: Check for undefined variables in Python files rg -l "snakemake" . | xargs ruff --select F821Length of output: 160
I'll run a new shell script to check if any occurrences of the
snakemakevariable remain in the updated file:
🏁 Script executed:
#!/bin/bash # Verify that the 'snakemake' variable is no longer present in the updated file rg -n "snakemake" harpy/scripts/demultiplex_gen1.pyLength of output: 619
Undefined "snakemake" variable still present in the file.
- The file still contains multiple instances of
snakemake(e.g., lines 83–95 and 119–120), meaning the review-requested improvements haven’t been applied.- The suggested replacement using command-line arguments (i.e., using
argsfrom an argument parser) is not yet implemented.🧰 Tools
🪛 Ruff (0.8.2)
83-83: Undefined name
snakemake(F821)
85-85: Undefined name
snakemake(F821)
86-86: Undefined name
snakemake(F821)
87-87: Undefined name
snakemake(F821)
88-88: Undefined name
snakemake(F821)
89-89: Undefined name
snakemake(F821)
90-90: Undefined name
snakemake(F821)
91-91: Undefined name
snakemake(F821)
92-92: Undefined name
snakemake(F821)
93-93: Undefined name
snakemake(F821)
94-94: Undefined name
snakemake(F821)
95-95: Undefined name
snakemake(F821)
108-108: Use a context manager for opening files
(SIM115)
109-109: Use a context manager for opening files
(SIM115)
119-119: Undefined name
snakemake(F821)
120-120: Undefined name
snakemake(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
harpy/_validations.py(1 hunks)harpy/demultiplex.py(7 hunks)harpy/scripts/demultiplex_gen1.py(1 hunks)harpy/snakefiles/demultiplex_gen1.smk(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/_validations.py
284-284: Undefined name file_path
(F821)
harpy/scripts/demultiplex_gen1.py
3-3: os imported but unused
Remove unused import: os
(F401)
49-49: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
63-63: Ambiguous variable name: I
(E741)
83-83: Undefined name snakemake
(F821)
85-85: Undefined name snakemake
(F821)
86-86: Undefined name snakemake
(F821)
87-87: Undefined name snakemake
(F821)
88-88: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Undefined name snakemake
(F821)
94-94: Undefined name snakemake
(F821)
95-95: Undefined name snakemake
(F821)
108-108: Use a context manager for opening files
(SIM115)
109-109: Use a context manager for opening files
(SIM115)
119-119: Undefined name snakemake
(F821)
120-120: Undefined name snakemake
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (8)
harpy/demultiplex.py (5)
31-31: LGTM!The docstring parameters section is correctly updated to include the new
--qx-rxoption.
44-44: LGTM!The
qx-rxoption is properly implemented with clear help text and correctly added to the function signature.Also applies to: 55-55
82-82: LGTM!The conda environments list is updated to include the new "demultiplex" environment.
92-92: LGTM!The
qx_rxflag is correctly added to the config dictionary.
108-114: LGTM!The workflow info presentation is nicely refactored using the new
workflow_infofunction, improving code maintainability.harpy/snakefiles/demultiplex_gen1.smk (3)
8-9: LGTM!The config variables are correctly initialized from the config dictionary.
38-70: LGTM!The rules are properly updated with:
- Correct output paths for barcode segments
- Proper input file handling
- Clear parameter passing
161-174: LGTM!The workflow summary is properly updated with input file information and correct terminology.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
harpy/scripts/demultiplex_gen1.py (3)
3-6: Remove unused import.The
osmodule is imported but never used in the code.-import os import sys import pysam from Levenshtein import distance🧰 Tools
🪛 Ruff (0.8.2)
3-3:
osimported but unusedRemove unused import:
os(F401)
43-59: Improve code readability and maintainability.Several issues need to be addressed:
- Magic number (999) should be replaced with a meaningful constant
- Variable names should follow Python naming conventions
- Unnecessary use of
.keys()def get_min_dist(needle, code_letter): - minDist = 999 - nbFound = 0 - minSeq = "" - for seq in bar_codes[code_letter].keys(): + MAX_DIST = float('inf') + min_dist = MAX_DIST + matches_found = 0 + min_seq = "" + for seq in bar_codes[code_letter]: d = distance(needle, seq) - if (d < minDist): - minDist = d - nbFound = 1 - minSeq = seq - elif (d == minDist): - nbFound += 1 - if (nbFound > 1): + if d < min_dist: + min_dist = d + matches_found = 1 + min_seq = seq + elif d == min_dist: + matches_found += 1 + if matches_found > 1: code_min_dist = f"{code_letter}00" else: - code_min_dist = bar_codes[code_letter][minSeq] + code_min_dist = bar_codes[code_letter][min_seq] return code_min_dist🧰 Tools
🪛 Ruff (0.8.2)
47-47: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
61-80: Improve code clarity and configurability.The function has several issues:
- Ambiguous parameter name 'I'
- Hardcoded barcode length (6)
- Inconsistent spacing around operators
-def get_read_codes(I, codeL, codeR): +def get_read_codes(index_seq, left_code, right_code): + """Extract and validate barcode sequences from the index read. + + Args: + index_seq: The index sequence to process + left_code: The expected left barcode segment + right_code: The expected right barcode segment + + Returns: + tuple: (right_code, left_code, status) + """ + BARCODE_LENGTH = 6 # Make protocol-dependent value configurable - left = I[0:6] # protocol-dependant - right = I[7:] + left = index_seq[0:BARCODE_LENGTH] + right = index_seq[7:] status = "found" - if left in bar_codes[codeL]: - lc = bar_codes[codeL][left] + if left in bar_codes[left_code]: + lc = bar_codes[left_code][left] else: - lc = get_min_dist(left, codeL) + lc = get_min_dist(left, left_code) status = "corrected" - if right in bar_codes[codeR]: - rc = bar_codes[codeR][right] + if right in bar_codes[right_code]: + rc = bar_codes[right_code][right] else: - rc = get_min_dist(right, codeR) + rc = get_min_dist(right, right_code) status = "corrected" - if (lc == f"{codeL}00" or rc == f"{codeR}00"): + if lc == f"{left_code}00" or rc == f"{right_code}00": status = "unclear" return rc, lc, status🧰 Tools
🪛 Ruff (0.8.2)
61-61: Ambiguous variable name:
I(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harpy/scripts/demultiplex_gen1.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/scripts/demultiplex_gen1.py
3-3: os imported but unused
Remove unused import: os
(F401)
47-47: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
61-61: Ambiguous variable name: I
(E741)
81-81: Undefined name snakemake
(F821)
83-83: Undefined name snakemake
(F821)
84-84: Undefined name snakemake
(F821)
85-85: Undefined name snakemake
(F821)
86-86: Undefined name snakemake
(F821)
87-87: Undefined name snakemake
(F821)
88-88: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Undefined name snakemake
(F821)
106-106: Use a context manager for opening files
(SIM115)
107-107: Use a context manager for opening files
(SIM115)
117-117: Undefined name snakemake
(F821)
118-118: Undefined name snakemake
(F821)
🔇 Additional comments (2)
harpy/scripts/demultiplex_gen1.py (2)
8-22: 🛠️ Refactor suggestionAdd input validation for nucleotide sequences.
The function should validate that sequences contain only valid nucleotides (ATCG).
def read_barcodes(file_path, segment): """Read and parse input barcode (segment) file of segment<tab>sequence""" data_dict = {} + valid_nucleotides = set('ATCG') with open(file_path, 'r') as file: for line in file: try: code, seq = line.rstrip().split() + if not set(seq.upper()).issubset(valid_nucleotides): + sys.stderr.write(f"Invalid nucleotides in sequence {seq} in {file_path}\n") + sys.exit(1) if code[0].upper() != segment: sys.stderr.write(f"Segments in {file_path} are expected to begin with {segment}, but begin with {code[0].upper()}\n") sys.exit(1) data_dict[seq] = code except ValueError: # skip rows without two columns continue return data_dictLikely invalid or redundant comment.
81-81: Verify Snakemake environment.The script assumes it's running within a Snakemake workflow. This should be verified to prevent runtime errors.
Add a check at the start of the script:
+if 'snakemake' not in globals(): + raise RuntimeError("This script is meant to be run within a Snakemake workflow.") + with open(snakemake.log[0], "w") as f:🧰 Tools
🪛 Ruff (0.8.2)
81-81: Undefined name
snakemake(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
harpy/_validations.py (2)
262-262: Make the regex pattern more specific.The current pattern allows any number of digits after the letter. Based on the error message in line 269, it should specifically match two digits.
- segment_pattern = re.compile(r'^[A-D]\d+$') + segment_pattern = re.compile(r'^[A-D]\d{2}$')
282-284: Improve error handling for malformed rows.The current implementation silently skips rows that don't have two columns. Consider providing feedback about malformed rows to help users identify and fix issues in their input files.
except ValueError: - # skip rows without two columns - continue + if line.strip() and not line.strip().startswith('#'): + print_error("malformed row", f"Row '{line.strip()}' does not have the expected format of 'sample<tab>segment'.") + print_solution("Each non-empty, non-comment row should have exactly two columns separated by a tab character.") + sys.exit(1) + continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harpy/_validations.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (2)
harpy/_validations.py (2)
258-291: LGTM! The overall implementation is solid.The function effectively validates the demultiplex schema file by:
- Ensuring segment IDs follow the correct format
- Preventing duplicate segment IDs
- Enforcing consistent code letters across all segments
- Providing clear error messages with solutions
289-289: 🛠️ Refactor suggestionUse print_error for consistency.
The error message uses
print_error, which is inconsistent with the error handling pattern used throughout the file.- print("invalid schema", f"Schema file {os.path.basename(infile)} has sample IDs expected to be identified across multiple barcode segments. All sample IDs for this technology should be in a single segment, such as [bold green]C[/bold green] or [bold green]D[/bold green].") + print_error("invalid schema", f"Schema file {os.path.basename(infile)} has sample IDs expected to be identified across multiple barcode segments ({', '.join(sorted(code_letters))}). All sample IDs for this technology should be in a single segment, such as [bold green]C[/bold green] or [bold green]D[/bold green].")Likely invalid or redundant comment.
Fix f string Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
harpy/scripts/demultiplex_gen1.py (1)
106-107: 🛠️ Refactor suggestionUse context managers for file handling.
File objects should be managed using context managers to ensure proper cleanup.
- R1_output = {sample: open(f"{outdir}/{sample}.R1.fq", 'w') for sample in samples} - R2_output = {sample: open(f"{outdir}/{sample}.R2.fq", 'w') for sample in samples} + class OutputFiles: + def __init__(self, outdir, samples): + self.files = { + 'R1': {sample: f"{outdir}/{sample}.R1.fq" for sample in samples}, + 'R2': {sample: f"{outdir}/{sample}.R2.fq" for sample in samples} + } + self.handles = {'R1': {}, 'R2': {}} + + def __enter__(self): + for read in ['R1', 'R2']: + for sample, path in self.files[read].items(): + self.handles[read][sample] = open(path, 'w') + return self.handles['R1'], self.handles['R2'] + + def __exit__(self, exc_type, exc_val, exc_tb): + for handles in self.handles.values(): + for f in handles.values(): + f.close() + + with OutputFiles(outdir, samples) as (R1_output, R2_output):Also applies to: 152-154
🧰 Tools
🪛 Ruff (0.8.2)
106-106: Use a context manager for opening files
(SIM115)
107-107: Use a context manager for opening files
(SIM115)
🧹 Nitpick comments (2)
harpy/scripts/demultiplex_gen1.py (2)
3-6: Remove unused import.The
osmodule is imported but never used in the code.-import os import sys import pysam from Levenshtein import distance🧰 Tools
🪛 Ruff (0.8.2)
3-3:
osimported but unusedRemove unused import:
os(F401)
81-82: Add type hints for snakemake workflow.Add type hints to help IDE and static analysis tools recognize snakemake workflow variables.
+from snakemake.script import Snakemake + +# Type hint for snakemake workflow +snakemake: Snakemake + with open(snakemake.log[0], "w") as f: sys.stderr = sys.stdout = f🧰 Tools
🪛 Ruff (0.8.2)
81-81: Undefined name
snakemake(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harpy/scripts/demultiplex_gen1.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/scripts/demultiplex_gen1.py
3-3: os imported but unused
Remove unused import: os
(F401)
47-47: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
61-61: Ambiguous variable name: I
(E741)
81-81: Undefined name snakemake
(F821)
83-83: Undefined name snakemake
(F821)
84-84: Undefined name snakemake
(F821)
85-85: Undefined name snakemake
(F821)
86-86: Undefined name snakemake
(F821)
87-87: Undefined name snakemake
(F821)
88-88: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
93-93: Undefined name snakemake
(F821)
106-106: Use a context manager for opening files
(SIM115)
107-107: Use a context manager for opening files
(SIM115)
117-117: Undefined name snakemake
(F821)
118-118: Undefined name snakemake
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (3)
harpy/scripts/demultiplex_gen1.py (3)
43-59: Improve code clarity and efficiency.
- Replace magic number with float('inf').
- Use clearer variable names.
- Remove unnecessary .keys() call.
def get_min_dist(needle, code_letter): - minDist = 999 - nbFound = 0 - minSeq ="" - for seq in bar_codes[code_letter].keys(): + min_dist = float('inf') + matches_found = 0 + min_seq = "" + for seq in bar_codes[code_letter]: d = distance(needle, seq) - if (d < minDist): - minDist = d - nbFound = 1 - minSeq= seq - elif (d == minDist): - nbFound += 1 - if (nbFound>1): + if d < min_dist: + min_dist = d + matches_found = 1 + min_seq = seq + elif d == min_dist: + matches_found += 1 + if matches_found > 1: code_min_dist = f"{code_letter}00" else: - code_min_dist = bar_codes[code_letter][minSeq] + code_min_dist = bar_codes[code_letter][min_seq] return code_min_dist🧰 Tools
🪛 Ruff (0.8.2)
47-47: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
61-79: Improve code maintainability and readability.
- Make protocol-dependent value configurable.
- Use descriptive parameter name.
- Add docstring.
-def get_read_codes(I, codeL, codeR): +def get_read_codes(sequence, left_code, right_code, barcode_length=6): + """Extract and validate barcode sequences from input read. + + Args: + sequence: Input sequence to extract barcodes from + left_code: Expected left barcode code letter + right_code: Expected right barcode code letter + barcode_length: Length of barcode sequence (default: 6) + + Returns: + tuple: (right_code, left_code, status) + """ - left = I[0:6] # protocol-dependant - right = I[7:] + left = sequence[0:barcode_length] + right = sequence[barcode_length + 1:] status = "found" - if left in bar_codes[codeL]: - lc = bar_codes[codeL][left] + if left in bar_codes[left_code]: + lc = bar_codes[left_code][left] else: - lc = get_min_dist(left, codeL) + lc = get_min_dist(left, left_code) status = "corrected" - if right in bar_codes[codeR]: - rc = bar_codes[codeR][right] + if right in bar_codes[right_code]: + rc = bar_codes[right_code][right] else: - rc = get_min_dist(right, codeR) + rc = get_min_dist(right, right_code) status = "corrected" - if (lc == f"{codeL}00" or rc == f"{codeR}00"): + if (lc == f"{left_code}00" or rc == f"{right_code}00"): status = "unclear" return rc, lc, status🧰 Tools
🪛 Ruff (0.8.2)
61-61: Ambiguous variable name:
I(E741)
8-22: 🛠️ Refactor suggestionAdd input validation and improve error handling.
- Add validation for nucleotides (ATCG only).
- Replace
sys.exit()with proper exception handling.def read_barcodes(file_path, segment): """Read and parse input barcode (segment) file of segment<tab>sequence""" data_dict = {} + valid_nucleotides = set('ATCG') with open(file_path, 'r') as file: for line in file: try: code, seq = line.rstrip().split() + if not set(seq.upper()).issubset(valid_nucleotides): + raise ValueError(f"Invalid nucleotides in sequence {seq} in {file_path}") if code[0].upper() != segment: - sys.stderr.write(f"Segments in {file_path} are expected to begin with {segment}, but begin with {code[0].upper()}\n") - sys.exit(1) + raise ValueError( + f"Segments in {file_path} are expected to begin with {segment}, " + f"but begin with {code[0].upper()}" + ) data_dict[seq] = code except ValueError: # skip rows without two columns continue return data_dictLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
harpy/snakefiles/align_strobealign.smk (1)
130-138: Remove redundantindex_duplicatesrule.This rule should be removed as the BAM indexing is now handled in the
assign_moleculesrule. The rule's presence is inconsistent with the changes inalign_bwa.smkand could lead to redundant operations.Apply this diff to remove the redundant rule:
-rule index_duplicates: - input: - outdir + "/samples/{sample}/{sample}.markdup.bam" - output: - temp(outdir + "/samples/{sample}/{sample}.markdup.bam.bai") - container: - None - shell: - "samtools index {input}"
♻️ Duplicate comments (1)
harpy/scripts/demultiplex_gen1.py (1)
105-106: 🛠️ Refactor suggestionUse context managers for file handling.
File handles should be managed using context managers to ensure proper cleanup.
🧰 Tools
🪛 Ruff (0.8.2)
105-105: Use a context manager for opening files
(SIM115)
106-106: Use a context manager for opening files
(SIM115)
🧹 Nitpick comments (2)
harpy/scripts/demultiplex_gen1.py (1)
119-132: Improve error handling and logging.Add error handling for file I/O operations and improve logging of read processing.
Apply this diff to improve error handling:
- for r1_rec, r2_rec, i1_rec, i2_rec in zip(R1, R2, I1, I2): + try: + for r1_rec, r2_rec, i1_rec, i2_rec in zip(R1, R2, I1, I2): + if not all([r1_rec, r2_rec, i1_rec, i2_rec]): + sys.stderr.write("Mismatched number of reads in input files\n") + sys.exit(1) + try: + segments['A'], segments['C'], statusR1 = get_read_codes(i1_rec.sequence, "C", "A", bar_codes) + segments['B'], segments['D'], statusR2 = get_read_codes(i2_rec.sequence, "D", "B", bar_codes) + except Exception as e: + sys.stderr.write(f"Error processing read: {e}\n") + continueharpy/_validations.py (1)
258-291: Improve documentation and error handling.The function needs:
- Better docstring with parameters and returns
- Consistent error message formatting
- Use of
print_errorinstead ofApply this diff to improve the code:
def validate_demuxschema(infile): - """Validate the file format of the demultiplex schema""" + """Validate the file format of the demultiplex schema. + + Args: + infile: Path to the schema file. + + Returns: + None + + Raises: + SystemExit: If validation fails with descriptive error message. + """ code_letters = set() #codes can be Axx, Bxx, Cxx, Dxx segment_ids = set() segment_pattern = re.compile(r'^[A-D]\d{2}$') with open(infile, 'r') as file: for line in file: try: sample, segment_id = line.rstrip().split() if not segment_pattern.match(segment_id): print_error("invalid segment format", f"Segment ID '{segment_id}' does not follow the expected format.") print_solution("This haplotagging design expects segments to follow the format of letter [green bold]A-D[/green bold] followed by [bold]two digits[/bold], e.g. [green bold]C51[/green bold]). Check that your ID segments or formatted correctly and that you are attempting to demultiplex with a workflow appropriate for your data design.") sys.exit(1) code_letters.add(segment_id[0]) if segment_id in segment_ids: print_error("ambiguous segment ID", "An ID segment must only be associated with a single sample.") print_solution_with_culprits( "A barcode segment can only be associated with a single sample. For example: [green bold]C05[/green bold] cannot identify both [green]sample_01[/green] and [green]sample_2[/green]. In other words, a segment can only appear once.", "The segment triggering this error is:" ) click.echo(segment_id) sys.exit(1) else: segment_ids.add(segment_id) except ValueError: # skip rows without two columns continue if not code_letters: print_error("incorrect schema format", f"Schema file {os.path.basename(infile)} has no valid rows. Rows should be sample<tab>segment, e.g. sample_01<tab>C75") sys.exit(1) if len(code_letters) > 1: - print("invalid schema", f"Schema file {os.path.basename(infile)} has sample IDs expected to be identified across multiple barcode segments. All sample IDs for this technology should be in a single segment, such as [bold green]C[/bold green] or [bold green]D[/bold green].") + print_error("invalid schema", f"Schema file {os.path.basename(infile)} has sample IDs expected to be identified across multiple barcode segments ({', '.join(sorted(code_letters))}). All sample IDs for this technology should be in a single segment, such as [bold green]C[/bold green] or [bold green]D[/bold green].") sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
harpy/_validations.py(1 hunks)harpy/scripts/demultiplex_gen1.py(1 hunks)harpy/snakefiles/align_bwa.smk(1 hunks)harpy/snakefiles/align_strobealign.smk(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/scripts/demultiplex_gen1.py
80-80: Undefined name snakemake
(F821)
82-82: Undefined name snakemake
(F821)
83-83: Undefined name snakemake
(F821)
84-84: Undefined name snakemake
(F821)
85-85: Undefined name snakemake
(F821)
86-86: Undefined name snakemake
(F821)
87-87: Undefined name snakemake
(F821)
88-88: Undefined name snakemake
(F821)
89-89: Undefined name snakemake
(F821)
90-90: Undefined name snakemake
(F821)
91-91: Undefined name snakemake
(F821)
92-92: Undefined name snakemake
(F821)
105-105: Use a context manager for opening files
(SIM115)
106-106: Use a context manager for opening files
(SIM115)
116-116: Undefined name snakemake
(F821)
117-117: Undefined name snakemake
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (3)
harpy/snakefiles/align_bwa.smk (1)
144-144: LGTM! Workflow optimization achieved.The integration of BAM indexing into the
assign_moleculesrule and marking the index as temporary streamlines the workflow by reducing the number of rules and properly managing intermediate files.harpy/snakefiles/align_strobealign.smk (1)
145-145: LGTM! Workflow optimization achieved.The integration of BAM indexing into the
assign_moleculesrule and marking the index as temporary streamlines the workflow by reducing the number of rules and properly managing intermediate files.harpy/scripts/demultiplex_gen1.py (1)
23-40: 🛠️ Refactor suggestionAdd input validation and handle edge cases.
The function needs to:
- Validate non-empty input files
- Validate segment ID format
- Handle empty code_letters set safely
Apply this diff to add validation:
def read_schema(file_path): """Read and parse schema file of sample<tab>id_segment""" data_dict = {} code_letters = set() + segment_pattern = re.compile(r'^[A-D]\d{2}$') with open(file_path, 'r') as file: + content = file.readlines() + if not content: + sys.stderr.write(f"Empty schema file: {file_path}\n") + sys.exit(1) for line in file: try: sample, segment_id = line.rstrip().split() + if not segment_pattern.match(segment_id): + sys.stderr.write(f"Invalid segment ID format: {segment_id}\n") + sys.exit(1) data_dict[segment_id] = sample code_letters.add(segment_id[0]) except ValueError: # skip rows without two columns continue + if not code_letters: + sys.stderr.write(f"No valid segment IDs found in {file_path}\n") + sys.exit(1) id_letter = code_letters.pop() return id_letter, data_dictLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
harpy/bin/assign_mi.py (1)
115-115: LGTM! Addition ofuntil_eof=Trueimproves read processing.The addition of
until_eof=Trueensures that all reads are processed, including those without coordinates. This is a good practice and aligns well with the existing unmapped read handling logic.This change makes the script more robust by ensuring no reads are accidentally skipped, which is particularly important for downstream analysis.
harpy/snakefiles/sv_naibr.smk (1)
25-25: Maintain consistent parameter naming.The configuration parameter is named
min_sizebut is mapped tomin_svin the args dictionary. Consider using consistent naming to improve code clarity.Apply this diff to maintain consistency:
- "min_sv" : min_size, + "min_size" : min_size,Also applies to: 38-38
harpy/snakefiles/sv_leviathan.smk (2)
44-59: Remove commented-out rule if no longer needed.The
process_alignmentsrule is commented out. If this rule is no longer needed, it should be removed rather than left as commented code.
29-29: Fix typo in parameter name.The parameter name contains a typo:
duplcates_threshshould beduplicates_thresh.Apply this diff to fix the typo:
-duplcates_thresh = config["variant_thresholds"]["duplicates"] +duplicates_thresh = config["variant_thresholds"]["duplicates"]harpy/sv.py (1)
9-9: Remove unused imports.The following imports are not used in the code:
LeviathanParamsNaibrParamsApply this diff to remove the unused imports:
-from ._cli_types_params import LeviathanParams, NaibrParams🧰 Tools
🪛 Ruff (0.8.2)
9-9:
._cli_types_params.LeviathanParamsimported but unusedRemove unused import
(F401)
9-9:
._cli_types_params.NaibrParamsimported but unusedRemove unused import
(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
harpy/bin/assign_mi.py(1 hunks)harpy/snakefiles/align_bwa.smk(0 hunks)harpy/snakefiles/align_strobealign.smk(0 hunks)harpy/snakefiles/sv_leviathan.smk(5 hunks)harpy/snakefiles/sv_leviathan_pop.smk(3 hunks)harpy/snakefiles/sv_naibr.smk(2 hunks)harpy/snakefiles/sv_naibr_phase.smk(2 hunks)harpy/snakefiles/sv_naibr_pop.smk(2 hunks)harpy/snakefiles/sv_naibr_pop_phase.smk(2 hunks)harpy/snp.py(5 hunks)harpy/sv.py(11 hunks)
💤 Files with no reviewable changes (2)
- harpy/snakefiles/align_strobealign.smk
- harpy/snakefiles/align_bwa.smk
🚧 Files skipped from review as they are similar to previous changes (1)
- harpy/snp.py
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/sv.py
9-9: ._cli_types_params.LeviathanParams imported but unused
Remove unused import
(F401)
9-9: ._cli_types_params.NaibrParams imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (9)
harpy/bin/assign_mi.py (1)
105-108: Clarify BAM index requirements.There's an inconsistency between the deprecation of index creation and the documentation. Line 25 still states "If bam, a matching index file should be in the same directory", but the index creation code is being deprecated.
Please clarify:
- Is a BAM index still required?
- If yes, how should users ensure the index exists?
- If no, should we update the help text to remove the index requirement?
Also applies to: 25-25
harpy/snakefiles/sv_naibr_phase.smk (1)
27-27: Maintain consistent parameter naming.The configuration parameter is named
min_sizebut is mapped tomin_svin the args dictionary. Consider using consistent naming to improve code clarity.Apply this diff to maintain consistency:
- "min_sv" : min_size, + "min_size" : min_size,Also applies to: 45-45
harpy/snakefiles/sv_naibr_pop.smk (1)
24-24: Maintain consistent parameter naming.The configuration parameter is named
min_sizebut is mapped tomin_svin the args dictionary. Consider using consistent naming to improve code clarity.Apply this diff to maintain consistency:
- "min_sv" : min_size, + "min_size" : min_size,Also applies to: 38-38
harpy/snakefiles/sv_leviathan_pop.smk (2)
23-29: LGTM! Configuration parameters are well-structured.The renaming of
min_svtomin_sizeimproves clarity, and the new variant threshold parameters provide granular control over variant size categories.
167-173: LGTM! Parameters are properly passed to LEVIATHAN.The parameters are correctly formatted and passed to the LEVIATHAN command, ensuring proper variant size categorization.
harpy/snakefiles/sv_naibr_pop_phase.smk (1)
29-29: LGTM! Configuration parameter is well-named.The renaming of
min_svtomin_sizeimproves clarity.harpy/sv.py (3)
58-60: LGTM! New command options are well-designed.The new options for duplicates and sharing thresholds are:
- Well-documented with clear descriptions
- Have appropriate type constraints
- Include sensible default values
118-123: LGTM! Variant thresholds configuration is well-structured.The dictionary structure for variant thresholds is:
- Clear and organized
- Properly typed
- Includes all necessary threshold categories
144-150: LGTM! Workflow info implementation is clean and consistent.The new implementation using
workflow_info:
- Improves code readability
- Maintains consistent formatting across workflows
- Properly handles optional parameters
Also applies to: 245-252
pdimens
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harpy/snakefiles/align_bwa.smk (1)
233-246: Add error handling for file operations.While the rule correctly handles file operations, it could benefit from error handling to ensure data integrity.
Consider this improvement:
shell: """ - mv {input.bam} {output.bam} - samtools index {output.bam} + if mv {input.bam} {output.bam}; then + samtools index {output.bam} || {{ echo "Failed to index BAM file"; exit 1; }} + else + echo "Failed to move BAM file" + exit 1 + fi """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
harpy/align.py(13 hunks)harpy/snakefiles/align_bwa.smk(6 hunks)harpy/snakefiles/align_strobealign.smk(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- harpy/align.py
- harpy/snakefiles/align_strobealign.smk
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (3)
harpy/snakefiles/align_bwa.smk (3)
21-21: LGTM! Configuration for optional barcode handling.The addition of
ignore_bxconfiguration aligns with the PR objectives to make barcode-related features optional.
120-120: LGTM! Proper implementation of conditional barcode handling.The changes correctly implement optional barcode handling by:
- Conditionally changing output paths
- Adding barcode mode parameter
- Updating the samtools markdup command
Also applies to: 124-125, 137-137
124-125: Verify implementation of QX and RX demux tags.The PR objectives mention making QX and RX demux tags optional with a new
--qx-rxflag, but this implementation only shows BX tag handling.Please confirm if the QX and RX demux tags are handled elsewhere or if they need to be added here. Let me help you search the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for QX and RX tag handling rg -i "qx|rx" --type pyLength of output: 1540
Verify QX/RX flag implementation across workflow components.
The search confirms that while the
align_bwa.smkfile only handles the BX tag, the QX/RX demux tags are implemented in the demultiplexing code (e.g., inharpy/demultiplex.pyandharpy/scripts/demultiplex_gen1.py), where the--qx-rxflag is defined and used. This is consistent with the design, as alignment-specific scripts need not manage these tags directly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
harpy/snakefiles/qc.smk (1)
22-22: Use.get()method with a default value for optional parameter.To prevent potential KeyError and maintain consistency with other optional parameters in the file, consider using the
.get()method.-ignore_bx = config["ignore_bx"] +ignore_bx = config.get("ignore_bx", False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
harpy/align.py(12 hunks)harpy/qc.py(5 hunks)harpy/snakefiles/qc.smk(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- harpy/align.py
🔇 Additional comments (7)
harpy/snakefiles/qc.smk (1)
199-199: LGTM! Barcode report is now correctly conditional.The condition properly combines both
skip_reportsandignore_bxflags to determine whether to include the barcode report, aligning with the PR objectives of making barcode-related features optional.harpy/qc.py (6)
13-13: LGTM!The import is correctly placed and follows Python's import style guidelines.
24-24: LGTM!The
--ignore-bxoption is correctly documented in the workflow controls section.
41-41: LGTM!The
--ignore-bxoption is well-defined with clear help text and follows the CLI options style.
96-96: LGTM!The
ignore_bxconfiguration is correctly added to pass the flag to the workflow.
119-126: LGTM!The workflow information presentation is well-structured and consistent with the standardization goals mentioned in the PR objectives.
46-46: Verify the handling of BX tags in dependent workflows.The function signature correctly includes the new
ignore_bxparameter. Since this makes BX tags optional, we should verify the impact on dependent workflows.Let's check for any workflows that might be affected by the BX tag changes:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for workflows that handle BX tags # Test: Look for BX-related code in workflow files rg -i '\bbx\b' --type pyLength of output: 14452
Action: BX Tag Workflow Handling Verified
The search results confirm that multiple workflows acknowledge the--ignore-bxoption and consistently handle BX tags—consistent with the changes made in theqcfunction signature. The dependent workflows in files likealign.py,metassembly.py, and others demonstrate proper handling of linked-read BX tags. There’s no evidence to suggest that the new optional behavior has a negative impact on these workflows.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
harpy/_cli_types_generic.py (1)
13-25: Improve validation logic and error handling.The validation logic should handle the case where
entriesisNone, allowing for an arbitrary number of integers.Apply this diff to improve the validation logic:
def convert(self, value, param, ctx): try: parts = [i.strip() for i in value.split(',')] - if len(parts) != self.entries: + if self.entries is not None and len(parts) != self.entries: raise ValueError for i in parts: try: int(i) except: raise ValueError return [int(i) for i in parts] except ValueError: - self.fail(f"{value} is not a valid list of integers. The value should be {self.entries} integers separated by a comma.", param, ctx) + expected = f"{self.entries} integers" if self.entries is not None else "a list of integers" + self.fail(f"{value} is not valid. The value should be {expected} separated by a comma.", param, ctx)🧰 Tools
🪛 Ruff (0.8.2)
21-21: Do not use bare
except(E722)
22-22: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (1)
harpy/_cli_types_generic.py (1)
6-8: Update docstring to reflect actual behavior.The current docstring states that the class accepts "an arbitrary number of integers", but the implementation enforces a specific number of entries when
entriesis provided.Apply this diff to update the docstring:
class IntList(click.ParamType): - """A class for a click type which accepts an arbitrary number of integers, separated by a comma.""" + """A class for a click type which accepts a comma-separated list of integers. + + Args: + entries (int, optional): If provided, enforces the exact number of integers required. + If None, accepts an arbitrary number of integers. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
harpy/_cli_types_generic.py(2 hunks)harpy/snakefiles/align_bwa.smk(6 hunks)harpy/snakefiles/align_strobealign.smk(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
harpy/_cli_types_generic.py
11-11: Undefined name max_entries
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build/Cache Container
🔇 Additional comments (8)
harpy/snakefiles/align_bwa.smk (4)
21-21: LGTM!The
ignore_bxconfiguration variable is properly added and used consistently throughout the workflow.
120-120: LGTM!The
mark_duplicatesrule has been properly updated to handle both BX and non-BX modes:
- Output path is conditionally modified based on
ignore_bx- New
bx_modeparameter correctly implements optional BX tag handlingAlso applies to: 124-125
233-246: LGTM!The new
index_bamrule is well-implemented:
- Correctly conditional on
ignore_bx- Properly handles BAM indexing when BX tags are ignored
- Uses appropriate container configuration
303-305: LGTM!The
workflow_summaryrule updates are correct:
- Reports collection is properly conditioned on both
skip_reportsandignore_bx- The
bx_modeparameter is correctly integratedAlso applies to: 309-309
harpy/snakefiles/align_strobealign.smk (4)
26-26: LGTM!The
ignore_bxconfiguration variable is consistently implemented across both alignment workflows.
111-111: LGTM!The
mark_duplicatesrule changes maintain consistency with the BWA workflow:
- Output path and
bx_modeparameter are handled identicallyAlso applies to: 115-116
224-238: LGTM!The
index_bamrule implementation is consistent with the BWA workflow:
- Same conditional logic
- Identical BAM indexing handling
295-296: LGTM!The
workflow_summaryrule changes maintain consistency with the BWA workflow:
- Reports collection and
bx_modeparameter are handled identicallyAlso applies to: 302-302
This PR:
None, making the row-adding more flexible than it was--qx-rxflag fixes Make writing QX and RX tags optional #195--duplicatesand--sharing-thresholdsfixes revisit sv leviathan cli options #196--min-svis now--min-sizein both leviathan and naibr workfows0as as special option to align--molecule-thresholdto ignore aligment distance for MI assignment fixes add0as special value for align molecule threshold #199--ignore-bxtoalign bwaandalign strobe, along withqcto process the data without linked-read considerations fixes add option to disable linked-read stuff within align bwa/strobe #193The original python code was provided by @BELKHIR and moderately modified to work in a Harpy workflow situation
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chore