-
Notifications
You must be signed in to change notification settings - Fork 37
fix: correct BED coordinate system calculation #103
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
Conversation
📝 WalkthroughWalkthroughCorrects BED interval length calculations across two files from inclusive end (end - start + 1) to half-open convention (end - start), aligning with BED 0-based coordinate system semantics in interval length and related position computations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
BED format uses 0-based, half-open intervals [start, end). Changed length calculation from `end - start + 1` to `end - start`. Files fixed: - regions_bed.c: length calculation and validation warning - dwgsim.c: 3 occurrences of region length calculation Closes #102 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
5007b63 to
e20b270
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dwgsim.c (1)
693-704: Remove the-1adjustment from line 697 after BED length correction.The position mapping formula on line 697 has an off-by-one error. With the corrected length calculation on line 695 (
j = end - start), the-1adjustment produces positions outside the valid BED region [start, end). Change line 697 from:pos = regions_bed->start[i] + pos - 1; // zero-basedto:
pos = regions_bed->start[i] + pos;
🧹 Nitpick comments (1)
src/regions_bed.c (1)
81-83: Validation check appears to be dead code.The condition
if(end - start != len)should never be true sincelen = end - startis assigned on line 55, and underflow is prevented by theend < startcheck on lines 72-75. While the update to match the new BED calculation is correct, this validation block appears to be dead code that could optionally be removed in a future cleanup.Optional: Remove unreachable validation
- if(end - start != len) { - fprintf(stderr, "Warning: len != end - start [%s,%u,%u,%u]\n", name, start, end, len); - } -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dwgsim.csrc/regions_bed.c
🔇 Additional comments (3)
src/dwgsim.c (2)
496-503: Correct BED interval length calculation.The change from
end - start + 1toend - startcorrectly implements BED's 0-based, half-open[start, end)convention. The inline comment clearly documents the semantics.
536-543: Correct BED interval length calculation for contig regions.The change correctly applies BED half-open semantics when accumulating region lengths for a specific contig, consistent with the fix at line 501.
src/regions_bed.c (1)
54-56: Correct BED interval length calculation.The change from
end - start + 1toend - startcorrectly implements BED's 0-based, half-open[start, end)convention. The added comment clearly documents this semantic.
Summary
BED format uses 0-based, half-open intervals [start, end).
Changed length calculation from
end - start + 1toend - start.Closes #102
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.