feat: added template.fixmate() (#83) #182
Conversation
TimD1
commented
Sep 3, 2024
TimD1
commented
- added fixmate() method
- added set_mate_info_on_supplementary() helper function
- moved builder._set_mate_info() to helper function set_mate_info()
- moved test_template_iterator.py to test_template.py (contained more than just iterator)
- added template_test_fixmate() tests
- added fixmate() method - added set_mate_info_on_supplementary() helper function - moved builder._set_mate_info() to helper function set_mate_info() - moved test_template_iterator.py to test_template.py (contained more than just iterator) - added template_test_fixmate() tests
TimD1
force-pushed
the
td_83_add-template-fixmate
branch
from
c0f71f5
to
dff494d
Compare
TimD1
temporarily deployed
to
github-actions-ci
GitHub Actions
Inactive
TimD1
temporarily deployed
to
github-actions-ci
GitHub Actions
Inactive
TimD1
temporarily deployed
to
github-actions-ci
GitHub Actions
Inactive
TimD1
temporarily deployed
to
github-actions-ci
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 89.21% 89.64% +0.42%
==========================================
Files 18 18
Lines 2068 2086 +18
Branches 457 460 +3
==========================================
+ Hits 1845 1870 +25
+ Misses 146 141 -5
+ Partials 77 75 -2 ☔ View full report in Codecov by Sentry. |
TimD1
changed the title
| # Arbitrarily set proper pair if the we have an FR pair with isize <= 1000 | ||
| if r1.is_reverse != r2.is_reverse and abs(r1.template_length) <= 1000: |
There was a problem hiding this comment.
issue This will break many structural variant callers.
Is there a reason to overwrite the proper pair status reported by the aligner?
There was a problem hiding this comment.
I'm not sure what initially motivated this; I transferred this _set_mate_info() helper function out of fgpyo/sam/builder.py. Are you aware of a reason for it to be present there?
There was a problem hiding this comment.
@msto what's the alternative? Invoking this function is tantamount to saying "I've changed some of the values on one or both of these reads, please make sure everything is in sync". I understand the concern around proper_pair - and it's true that this implementation made sense when it was in SamBuilder, and less so here...
But if someone has changed alignment values .. the value from the aligner is also questionable here.
There was a problem hiding this comment.
@TimD1 - @msto and I chatted and we think the right solution here is a little complicated. We suggest doing something like:
def set_mate_info(
r1: AlignedSegment,
r2: AlignedSegment,
is_proper_pair: Callable[[AlignedSegment, AlignedSegment], bool] = lambda a, b: a.is_proper_pair and b.is_proper_pair
) -> Unit
...I.e. the default is to leave it True if it's already set on R1 and R1, but set it to false if already false, or there is disagreement. Then a user (e.g. SamBuilder) can provide it's own lambda.
