Merge Claudiu Craciun and Alan Kuznicki's semester project work into main - #5
Draft
alankuznicki wants to merge 40 commits into
Draft
Merge Claudiu Craciun and Alan Kuznicki's semester project work into main#5alankuznicki wants to merge 40 commits into
alankuznicki wants to merge 40 commits into
Conversation
added 24 commits
March 24, 2026 00:36
alankuznicki
commented
Aug 7, 2026
Comment on lines
+148
to
+182
| # --- Correct behavior (current) --- | ||
| # Fraction of bases in [start, start+extent) that are actually | ||
| # unmappable, computed once for the whole chromosome via a prefix | ||
| # sum over the mappability track, then sliced per candidate window. | ||
| frac = _unmappable_fraction(mappability_track, seq_starts, extent) | ||
| seq_starts = seq_starts[frac <= unmap_threshold] | ||
|
|
||
| # --- Old (buggy) behavior, kept here for reference --- | ||
| # The original filter_idx_by_unmap_threshold (data_bed.py) computed, | ||
| # per window, `overlap_length = sum(overlaps.end - overlaps.start)` | ||
| # using the RAW, unclamped length of every unmap interval that | ||
| # touched the window at all -- even when only a sliver of that | ||
| # interval actually fell inside the window. So a single long | ||
| # unmappable region (e.g. 60bp) could cause a window that only | ||
| # overlapped it by a handful of bp to be dropped entirely, because | ||
| # the full 60bp counted against the threshold budget instead of the | ||
| # true (much smaller) overlap. This was a bug, not an intentional | ||
| # design choice -- verified against the original by differential | ||
| # testing (see conversation/PR notes). If bit-identical window | ||
| # selection against an old cached index or trained checkpoint is | ||
| # ever needed, this reproduces the dominant effect of that bug | ||
| # (raw interval length, not clipped overlap) without reintroducing | ||
| # the original's slow per-window pandas loop -- it loops over | ||
| # unmap intervals instead, which are typically far fewer than | ||
| # candidate windows. Uncomment in place of the two lines above: | ||
| # | ||
| # unmap_df = pd.read_csv(unmappable_bed_file, delimiter='\t', header=None, | ||
| # names=['chr', 'start', 'end']) | ||
| # unmap_df = unmap_df[(unmap_df.chr == f'chr{chrom}') | (unmap_df.chr.astype(str) == f'{chrom}')] | ||
| # window_end = seq_starts + extent | ||
| # raw_overlap_length = np.zeros(len(seq_starts), dtype=np.int64) | ||
| # for gs, ge in zip(unmap_df.start.to_numpy(), unmap_df.end.to_numpy()): | ||
| # touches = (seq_starts < ge) & (gs < window_end) | ||
| # raw_overlap_length[touches] += (ge - gs) # bug: full interval length, not clipped | ||
| # seq_starts = seq_starts[raw_overlap_length <= unmap_threshold * extent] |
Collaborator
Author
There was a problem hiding this comment.
Claude said that this was a genuine behavior-affecting bug in the original code. Thoughts?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO