feat(ceiling): disjoint-split depth extrapolation (replaces bootstrap)#243
feat(ceiling): disjoint-split depth extrapolation (replaces bootstrap)#243beabevi wants to merge 1 commit into
Conversation
Replace the bootstrap self-split in compute_ceiling with a disjoint (without-replacement) split at multiple depths, extrapolated to full depth. The bootstrap drew both halves from the same cells, so they were not independent, which biased the ceiling in both directions (not a reliable upper bound): the shared cells make the halves over-agree (inflating it), while the duplicate cells over-call the FDR-gated DE metrics and drag the recovery metrics down. A disjoint split is unbiased but shallow (each half <= n/2); measuring each metric across depths and extrapolating to full depth corrects for it. For reliability-like (correlation) metrics this extrapolation reduces to the analytical Spearman-Brown correction (verified: extrap matches SB on those metrics); it generalizes the same idea to metrics with no closed form. - remove _bootstrap_halves; add _disjoint_halves + a per-metric depth-curve fit - compute_ceiling returns one per-metric table (was a (results, agg_results) tuple); writes a single ceiling_results.csv (was ceiling_results.csv + agg_ceiling_results.csv) - CLI --ceiling help, README, CLAUDE updated; add tests/test_ceiling_extrap.py
There was a problem hiding this comment.
Code Review
This pull request refactors the data ceiling estimation in cell-eval to use a disjoint-split depth extrapolation method instead of a bootstrap self-split, which avoids bias in the ceiling estimates. The changes update the CLI, documentation, MetricsEvaluator class, and unit tests. The review feedback highlights three opportunities to improve robustness: validating that the fracs parameter is not empty to prevent a crash in min(), guarding against empty index lists in np.concatenate during disjoint splitting, and filtering out floating-point NaN values during metric aggregation to prevent downstream extrapolation failures.
| fracs = tuple(sorted(set(fracs), reverse=True)) | ||
| if any(f <= 0.0 or f > 1.0 for f in fracs): | ||
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") |
There was a problem hiding this comment.
If fracs is empty, min(fracs) on line 206 will raise a ValueError: min() arg is an empty sequence. Explicitly validating that fracs is not empty prevents this crash.
| fracs = tuple(sorted(set(fracs), reverse=True)) | |
| if any(f <= 0.0 or f > 1.0 for f in fracs): | |
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") | |
| fracs = tuple(sorted(set(fracs), reverse=True)) | |
| if not fracs: | |
| raise ValueError("fracs must not be empty") | |
| if any(f <= 0.0 or f > 1.0 for f in fracs): | |
| raise ValueError(f"fracs must be in (0, 1]; got {fracs}") |
| # Disjoint split has no duplicate rows, so obs names stay unique. | ||
| half_real = real[np.concatenate(a_idx)].copy() | ||
| half_pred = real[np.concatenate(b_idx)].copy() |
There was a problem hiding this comment.
If no perturbations have enough cells to split at the given frac (e.g., if eligible is empty or cell counts are too low), a_idx and b_idx will be empty. Calling np.concatenate on an empty list raises a ValueError. Guarding against this with a descriptive error improves robustness.
# Disjoint split has no duplicate rows, so obs names stay unique.
if not a_idx:
raise ValueError(
f"No perturbations had enough cells to split at frac={frac}. "
"Ensure your dataset has sufficient cells per perturbation."
)
half_real = real[np.concatenate(a_idx)].copy()
half_pred = real[np.concatenate(b_idx)].copy()| arr = results[col].drop_nulls().to_numpy() | ||
| if arr.size == 0: | ||
| continue | ||
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) |
There was a problem hiding this comment.
Polars' drop_nulls() drops missing values (null), but does not drop floating-point NaN values. If any metric computation returns NaN for a perturbation, np.median or np.mean will propagate the NaN, causing the subsequent curve fitting/extrapolation to fail. Filtering out non-finite values (including NaN and inf) ensures robust aggregation.
| arr = results[col].drop_nulls().to_numpy() | |
| if arr.size == 0: | |
| continue | |
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) | |
| arr = results[col].drop_nulls().to_numpy() | |
| arr = arr[np.isfinite(arr)] | |
| if arr.size == 0: | |
| continue | |
| out[col] = float(np.median(arr) if agg == "median" else np.mean(arr)) |
|
can you bump the semver as this changes behavior: version = "0.8.1" -> version = "0.8.2" |



Summary
Reworks the
--ceilingdata ceiling (MetricsEvaluator.compute_ceiling) from the bootstrap self-split to a disjoint-split depth extrapolation. For each perturbation the real data is split without replacement into two halves at several depths (fracs=(1.0, 0.5, 0.25)), the metric suite is run on each self-split, and every metric's value-vs-depth curve is extrapolated to full depth. The result is a per-metric, unbiased estimate of the best score any model could reach given the noise in the real data.Why
The bootstrap drew both halves from the same cells (
2ndraws with replacement, then split), so they weren't independent. That biases the ceiling in both directions, so it isn't a reliable upper bound:A disjoint (without-replacement) split gives genuinely independent halves (unbiased), at the cost of depth (each half ≤
n/2). Measuring each metric across several depths and extrapolating to full depth corrects for that depth cost.Relationship to Spearman-Brown: for reliability-like (correlation) metrics this extrapolation is the Spearman-Brown correction: SB (
2r/(1+r)) is its analytical closed form. The extrapolation generalizes the same idea to metrics that have no closed form (the DE recovery metrics), which is why it's used as the single method rather than SB.What changed
_bootstrap_halves; add_disjoint_halves(disjoint split at a depth fraction) + a per-metric depth-curve fit (attenuation/reliability form, linear fallback), extrapolated to full depth.compute_ceilingreturns one per-metric table (measure, withextrap_linear/resid/modeldiagnostics) instead of the old(results, agg_results)tuple.ceiling_results.csv(wasceiling_reslts.csv).--ceilingCLI help,README,CLAUDE.md). The existing ceiling tests intests/test_eval.pywere updated for the new API, and a newtests/test_ceiling_extrap.pywas added (unit tests for the extrapolation math + a disjoint-split check).pytest,ruff, andty(on the changed files) pass.