fix: Check against HN tag instead of is_unmapped property
#69
Conversation
There was a problem hiding this comment.
I think there needs to be a further change here, or at least a clear explanation/justification of why not. I think when the record is marked as unmapped, what that really means is that bwa believes the primary alignment to be erroneous - and as such I think to_hits() should ignore the primary alignment and only decode the secondary alignments.
WalkthroughThe changes in the pull request primarily focus on the Assessment against linked issues
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 (1)
prymer/offtarget/bwa.py (1)
159-162: Remove unnecessary whitespace in docstring blank lineThe blank line at line 161 contains whitespace. Please remove the extra whitespace to adhere to PEP 8 style guidelines.
🧰 Tools
🪛 Ruff
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prymer/offtarget/bwa.py (4 hunks)
🧰 Additional context used
🪛 Ruff
prymer/offtarget/bwa.py
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (2)
prymer/offtarget/bwa.py (2)
363-373: Logic for adjusting hit counts is correctThe updated logic in lines 363-373 appropriately adjusts
num_hitsandhits, ensuring that theBwaResultaccurately reflects the total number of hits even when the number of hits exceedsmax_hits.
418-422: Correctly filter out hits that extend beyond contig boundariesThe added filtering in lines 418-422 effectively removes artifacts from
bwa aln's alignment process by excluding hits that extend beyond the end of a contig, enhancing the accuracy of the results.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prymer/offtarget/bwa.py (1)
161-161: Remove whitespace on blank lineThere's trailing whitespace on the blank line at line 161. This can lead to style inconsistencies.
Apply this diff to remove the whitespace:
- +🧰 Tools
🪛 Ruff
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/tests.yml (0 hunks)
- prymer/offtarget/bwa.py (4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🪛 Ruff
prymer/offtarget/bwa.py
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (2)
prymer/offtarget/bwa.py (2)
363-376: Logic correctly utilizesHNtag for hit countThe updated logic appropriately uses the
HNtag to determine the number of hits, aligning with the PR objectives. It correctly handles cases where the number of hits is zero, between zero andself.max_hits, and greater than or equal toself.max_hits.
421-424: Properly filters out hits spanning contig boundariesThe addition ensures that hits extending beyond the end of a contig are removed, addressing artifacts from
bwa aln's alignment process. This enhances the accuracy of the returned hits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/offtarget/test_bwa.py (1)
265-282: Approve changes with a minor suggestion for improvementThe modifications to
test_to_result_num_hits_on_unmappedeffectively address the PR objectives by testing different scenarios for unmapped reads using the HN tag. The test now covers cases where HN is non-zero, zero, and absent, which aligns with the goal of improving the handling of multiply mapped reads.Consider adding brief comments explaining the expected behavior for each scenario to enhance test readability. For example:
# Scenario 1: HN tag is non-zero (multiply mapped read) rec = SamBuilder().add_single(name=query.id, attrs={"HN": 42}) result = bwa._to_result(query=query, rec=rec) assert result.hit_count == 42 # Hit count should match HN value assert result.hits == [] # Hits list should be empty for unmapped reads # Scenario 2: HN tag is zero (uniquely mapped read) ... # Scenario 3: HN tag is absent (fallback behavior) ...This addition would make the test's intentions clearer and help future maintainers understand the expected behavior for each case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prymer/offtarget/bwa.py (4 hunks)
- tests/offtarget/test_bwa.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prymer/offtarget/bwa.py
🧰 Additional context used
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 96.77% 96.76% -0.01%
==========================================
Files 26 26
Lines 1704 1701 -3
Branches 327 326 -1
==========================================
- Hits 1649 1646 -3
Misses 29 29
Partials 26 26 ☔ View full report in Codecov by Sentry. |
Closes #68
bwa alnproduces inconsistent behavior when recording the mapped status of multiply mapping reads. In most cases, such reads are flagged as mapped with a mapq of 0. However, we have discovered some cases where these reads are flagged as unmapped.prymercurrently relies on the unmapped flag to identify reads with a hit count of zero. This has been yielding errors when attempting to parse reads in the latter category. This information can instead be obtained directly from theHNtag.NB: Thanks to detective work by @tfenne
This PR additionally discards any hits that extend beyond the end of the contig to which they are mapped.