-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue with wrong values for overlapping bed-ranges #84
Conversation
d4/src/d4file/track.rs
Outdated
@@ -83,7 +88,12 @@ where | |||
if let Some(handle) = handle { | |||
handle.init(); | |||
if let Some(top) = active_heap.first() { | |||
let this_begin = top.get_range().0; | |||
let this_begin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let this_begin = top.get_range().0.max(last_end)
d4/src/d4file/track.rs
Outdated
@@ -70,7 +70,12 @@ where | |||
} | |||
|
|||
if this_end != last_end { | |||
let this_begin = top.get_range().0; | |||
let this_begin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let this_begin = top.get_range().0.max(last_end)
Thank you for the review @brentp ! Makes much sense to do more careful testing. I added a more extensive case and simplified the code as suggested. Ready for re-review. |
As a part of testing #83, we realized that some values were used multiple times in calculations.
Tracking this down, it seems that in
track.rs
, same ranges were used multiple times. This is the part of the code I recently updated to fix a separate issue, and might have been a new bug caused by solving the old.I have prepared a 10nt
d4
file mapped with two reads to test this:Expected total coverage would be
(3 * 2 + 3 * 1) / 10 = 0.9
Before the update:
After the change:
I have prepared a unit test based on this as part of the PR. This and other tests are succeeding, except two
remote-view
tests (which I don't think are related to this change).Would you be able to review @brentp ?