Skip to content

Fix partition aggregation and wire median_neighbor_distance - #128

Closed
sanghoonio wants to merge 2 commits into
genom_distfrom
fix/median-neighbor-distance
Closed

Fix partition aggregation and wire median_neighbor_distance#128
sanghoonio wants to merge 2 commits into
genom_distfrom
fix/median-neighbor-distance

Conversation

@sanghoonio

@sanghoonio sanghoonio commented Sep 3, 2026

Copy link
Copy Markdown
Member

genom_dist currently fails all four checks: aggregation.py has a syntax error, so import bbconf fails and no tests are collected. This fixes that, plus one other bug found while looking into it.

Two commits, independent of each other — the first is just the CI unblock.


1. The module doesn't parse — f31557f

5edc86f (Copilot Autofix) removed agg_columns.extend( and its [ from _aggregate_partitions, leaving a dangling list:

agg_columns = []
for name, column in _PARTITION_COLUMNS:
            func.avg(column).label(f"{name}_mean"),   # ← extend([ was here

bedsets.py and bedfiles.py both import this module, so nothing in bbconf imports. a8de95d and fa444b7 were both green; 5edc86f is where it went red.

Restored the wrapper.

The same commit made partition values 100× too small

It also dropped the * 100, acting on a review comment saying the *_percentage columns "appear to already be stored as percentages (0–100)".

They're fractions:

where what it stores
regionstat.R:212 Freq / length(query)
gtars_backend.py:227 count / total
bedbase-ui selection-stats.tsx:11 renders them as value * 100

Easy to get wrong from inside bbconf — the column name, the old docstring, and the test fixture all suggest otherwise, and all three sources above live in other repos.

What I did: the missing * 100 is only a bug because the keys are named _pct. So rather than putting the multipliers back, I renamed the keys — mean_pct/sd_pct become mean/sd, and partitions now returns fractions like everything else in BedSetDistributions.

Consumers scale for display. bedbase-ui does this at the plot layer (b8a2464 on master), next to the axis label and matching how the local comparison path already works. Nothing was deployed against the old keys.

The docstring now records where the fraction comes from, so the next reader has the evidence to hand.

2. median_neighbor_distance never had a value — 5b35653

_SCALAR_COLUMNS read it from BedStats.tssdist. Nothing writes that column — it appears twice in the whole tree (initial migration, ORM declaration), and BedStatsModel doesn't declare the field, so extra="ignore" drops the value bedboss computes before it can be stored. count() returned 0, and the key was silently omitted from every response.

Added a real column, plus the BedStatsModel field and a migration. Both edits are needed together: with only the model field every insert raises TypeError; with only the column, pydantic keeps dropping the value.

Dropped tssdist rather than renaming it. It's a leftover TSS-distance column superseded by median_tss_dist (both created by the initial migration). Neighbor distance is a different measurement — the gap between consecutive regions within a file, not the distance to the nearest annotated TSS — so a rename would relabel stale TSS values and average them into bedset stats.

Migration chains off c7f3a9e1b204, the only head on this branch.


Worth checking before merge

  • SELECT count(*) FROM bed_stats WHERE tssdist IS NOT NULL; — should be 0, since no writer has ever existed. If it isn't, that's real data being discarded.
  • mean_pctmean is a response-shape change. bedbase-ui is the only consumer I could find, and it's updated.
  • docs/schema.svg still shows tssdist and needs regenerating against a live DB.

Verified

Lint and format pass. Single alembic head; upgrade and downgrade both render correctly offline. ORM metadata matches the migration. Partition and scalar SQL compile against PostgreSQL with no reference to tssdist. median_neighbor_distance added to both test fixtures, so test_upload exercises the model↔ORM coupling.

Not verified: the DB-backed tests. No Postgres available where I was working, so CI is the first real run.

Left alone

Noticed but not touched — happy to open separate PRs:

  • per-chrom n in _aggregate_region_distribution is taken from the first row only
  • tss_histogram docstring says "summed"; the code computes AVG/SD
  • get_distributions() fallback reports n_files=0 for bedsets that do have members
  • get_batch docstring claims one round-trip; the two selectinloads make it three
  • get(full=True) still returns the distributions blob

🤖 Generated with Claude Code

https://claude.ai/code/session_01PRGhWjGiUXFoygd35QsKQ7

sanghoonio and others added 2 commits September 3, 2026 10:49
5edc86f removed the `agg_columns.extend([` wrapper along with the `* 100`
multipliers, leaving an orphaned list literal. aggregation.py has not parsed
since, and both bedsets.py and bedfiles.py import it, so `import bbconf`
fails outright — conftest.py cannot load and no tests are collected.

Restore the wrapper. The multipliers stay off, but for a different reason
than the autofix assumed: the bed_stats.*_percentage columns hold a fraction,
not a percentage. Both producers divide by the region count —
regionstat.R:212 stores Freq/length(query), gtars_backend.py:227 stores
count/total — and bedbase-ui renders the per-file values as value * 100.

Since the aggregation no longer rescales, `mean_pct`/`sd_pct` would have been
misnamed, so they become `mean`/`sd`. That also matches the shape already used
by `scalar_summaries`, leaving every value in BedSetDistributions on the same
scale as the column it aggregates. bedbase-ui scales to percent at the plot
layer, where the local comparison path already does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRGhWjGiUXFoygd35QsKQ7
The aggregation surfaced `median_neighbor_distance` by reading
`bed_stats.tssdist`, but nothing writes that column. It has two references in
the whole tree — the initial migration and the ORM declaration — and
BedStatsModel has never declared the field, so with `extra="ignore"` the value
bedboss computes (gtars_backend.py:203-217) is dropped before it reaches
`BedStats(**stats.model_dump())`. `count()` returned 0, `if not n: continue`
fired, and the key was silently absent from every scalar_summaries payload.

Add a real `median_neighbor_distance` column and drop `tssdist`. The ORM
attribute and the BedStatsModel field have to land together: with only the
model field, `model_dump()` emits a key `BedStats.__init__` rejects and every
insert raises TypeError; with only the ORM attribute, pydantic keeps dropping
the value.

`tssdist` is dropped rather than renamed because it is a vestige of the *TSS*
quantity, superseded by `median_tss_dist` (both were created by the initial
migration). Neighbor distance is a different measurement — the gap between
consecutive regions within a file, versus the distance from each region to the
nearest annotated TSS — so renaming would relabel any stale TSS values and
average them into bedset scalar_summaries.

The revision chains off c7f3a9e1b204, which is the only head on this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRGhWjGiUXFoygd35QsKQ7
@sanghoonio

Copy link
Copy Markdown
Member Author

Superseded — pushed the fix directly to genom_dist as 2fd21d3, so it's part of #127 now.

Dropped the median_neighbor_distance schema commit from scope. That work is preserved on fix/median-neighbor-distance (5b35653) if it's wanted later.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PRGhWjGiUXFoygd35QsKQ7

@sanghoonio sanghoonio closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant