Expose some bwa indel parameters through the off-target checker. #95
Conversation
|
@ ameynert for your consideration |
|
Warning Rate limit exceeded@tfenne has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 48 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 (3)
WalkthroughThe pull request introduces updates to the In the 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: 2
🧹 Outside diff range and nitpick comments (1)
prymer/offtarget/offtarget_detector.py (1)
206-211: Enhance gap parameter documentationAdd context about indel impact on primer annealing and define "long gaps".
max_gap_opens: the maximum number of gaps (insertions or deletions) allowable in an - alignment of a oligo to the reference + alignment of a oligo to the reference. Indels within 3bp of primer ends + significantly affect primer annealing. max_gap_extends: the maximum number of gap extensions allowed; extending a gap beyond a single base costs 1 gap extension. Can be set to -1 to allow - unlimited extensions up to max diffs (aka max mismatches), while disallowing - "long gaps". + unlimited extensions up to max diffs (aka max mismatches). Long gaps + (>1bp) near primer ends may prevent proper annealing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
prymer/offtarget/bwa.py(3 hunks)prymer/offtarget/offtarget_detector.py(3 hunks)
🔇 Additional comments (2)
prymer/offtarget/bwa.py (2)
235-235: LGTM: Parameter addition and documentation are clear.
Parameter name and default value align with experimental findings about indel effects on primer annealing.
Also applies to: 252-253
294-295: Verify BWA flag usage.
Confirm -i is the correct BWA flag for min_indel_to_end_distance.
| max_gap_opens: int = 0, | ||
| max_gap_extends: int = -1, | ||
| max_amplicon_size: int = 1000, |
There was a problem hiding this comment.
Default max_gap_opens=0 is too restrictive
PR indicates indels affect primer annealing. Consider default of 1 to allow single indels.
- max_gap_opens: int = 0,
+ max_gap_opens: int = 1,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| max_gap_opens: int = 0, | |
| max_gap_extends: int = -1, | |
| max_amplicon_size: int = 1000, | |
| max_gap_opens: int = 1, | |
| max_gap_extends: int = -1, | |
| max_amplicon_size: int = 1000, |
There was a problem hiding this comment.
@tfenne any reason why not? Or should we keep this at 0 for backwards compatibility?
There was a problem hiding this comment.
I wanted to keep zero for back-compat for now.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage 96.70% 96.70%
=======================================
Files 26 26
Lines 1728 1728
Branches 193 193
=======================================
Hits 1671 1671
Misses 31 31
Partials 26 26 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/offtarget/test_bwa.py (1)
Line range hint
218-240: Add test cases for indel distance behaviorCurrent tests don't verify indel distance constraints.
Add test cases:
@pytest.mark.parametrize( "indel_distance, should_map", [ (2, False), # Too close to end (3, True), # Minimum allowed distance (4, True), # Above minimum ] ) def test_indel_distance_constraints(ref_fasta: Path, indel_distance: int, should_map: bool) -> None: bases = _PERFECT_BASES[:indel_distance] + "TTT" + _PERFECT_BASES[indel_distance:] with BwaAlnInteractive(ref=ref_fasta, max_hits=1, min_indel_to_end_distance=3) as bwa: result = bwa.map_one(query=bases, id="test") assert (result.hit_count > 0) == should_map
| max_gap_opens: int = 0, | ||
| max_gap_extends: int = -1, | ||
| max_amplicon_size: int = 1000, |
There was a problem hiding this comment.
@tfenne any reason why not? Or should we keep this at 0 for backwards compatibility?
tfenne
force-pushed
the
tf_expose_gap_limits
branch
from
1f1ca9e to
fca5781
Compare
I did some playing with ntthal, and it appears to that the when calculating Tms of duplexes, an indel closer than 3bp from the end of the primer causes it to see the rest of the primer as non-annealed. In some cases it can be more (depending on sequence, it may not see the remainder as annealed after an indel 4-5bp in), but it seems consistent that it needs at least 3bp past the indel to anneal bases. Hence setting
min_indel_to_end_distance: int = 3.