Allow ceiling-only evaluation without a prediction - #245
Conversation
`cell-eval run` previously required `--adata-pred`. The data ceiling is estimated from the real data alone (disjoint self-split), so a prediction is unnecessary when only the ceiling is wanted. - `--adata-pred` is now optional; omitting it (together with `--ceiling`) computes only the real-data ceiling. Omitting it without `--ceiling` errors. - `MetricsEvaluator` accepts `adata_pred=None` (ceiling-only mode): skips the main `de_comparison`, and `compute()` raises directing to `compute_ceiling()`. - `_build_anndata_pair` mirrors real into pred when no prediction is given (the placeholder is never scored; the ceiling reads only `.real`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a "ceiling-only" mode to the evaluation pipeline, allowing users to estimate the real-data ceiling without providing a predicted AnnData object (adata_pred). The CLI and MetricsEvaluator have been updated to handle adata_pred as optional, bypassing prediction-based computations and enforcing that --ceiling is specified when running in this mode. The review feedback highlights a misleading CLI help message and recommends raising explicit validation errors if precomputed DE results are provided in ceiling-only mode, as they are currently silently ignored.
| ceiling_only = args.adata_pred is None | ||
| if ceiling_only and not args.ceiling: | ||
| raise ValueError( | ||
| "--adata-pred is required unless --ceiling is passed (ceiling-only mode)." | ||
| ) |
There was a problem hiding this comment.
In ceiling-only mode, any precomputed DE results passed via --de-pred or --de-real will be silently ignored because the main evaluation is skipped and the ceiling estimation computes its own DE on the split halves. To prevent user confusion and ensure correctness, we should raise a ValueError if these arguments are provided in ceiling-only mode.
| ceiling_only = args.adata_pred is None | |
| if ceiling_only and not args.ceiling: | |
| raise ValueError( | |
| "--adata-pred is required unless --ceiling is passed (ceiling-only mode)." | |
| ) | |
| ceiling_only = args.adata_pred is None | |
| if ceiling_only: | |
| if not args.ceiling: | |
| raise ValueError( | |
| "--adata-pred is required unless --ceiling is passed (ceiling-only mode)." | |
| ) | |
| if args.de_pred is not None or args.de_real is not None: | |
| raise ValueError( | |
| "Precomputed DE results (--de-pred or --de-real) cannot be used in ceiling-only mode " | |
| "as the ceiling is estimated by splitting the real data and re-computing DE on the halves." | |
| ) |
| # alone, so no prediction is required. When adata_pred is None we skip | ||
| # building the main de_comparison (compute() is unavailable) - only | ||
| # compute_ceiling() may be called. | ||
| self.ceiling_only = adata_pred is None |
There was a problem hiding this comment.
When MetricsEvaluator is instantiated programmatically in ceiling-only mode (adata_pred=None), any provided de_pred or de_real arguments are silently ignored. We should raise a ValueError to prevent programmatic API misuse and make this limitation explicit.
self.ceiling_only = adata_pred is None
if self.ceiling_only and (de_pred is not None or de_real is not None):
raise ValueError(
"de_pred and de_real cannot be provided in ceiling-only mode (adata_pred=None)."
)
LeonHafner
left a comment
There was a problem hiding this comment.
Design is right — the ceiling reads only .real, so requiring a prediction was an artificial barrier. I merged this onto current main locally and it composes cleanly. Notes below.
Blockers
- No tests for the new mode. The suite is
48 passedboth with and without this PR, so nothing exercises ceiling-only. Worth covering:compute_ceiling()withadata_pred=None,compute()raising, and the CLI refusal when-apand--ceilingare both omitted. - CI is stale — needs a rebase. Checks ran Jul 24 on
b53dde6; #246 merged Jul 27.pull_requestbuilds test the merge commit, so these green ticks validated amainwithout ther > 0ceiling guard, and CI is not re-run when the base moves.
Should fix
- CLI misuse
raise ValueErrorgives a traceback and exit 1;parser.error()gives a usage message and exit 2. It is the first thing a user hits on a wrong invocation. pred = realaliases the same object (pair.real is pair.predisTrue). Harmless now sincecompute()is blocked and the halves are fresh copies, but a future write to.predwould silently corrupt.real— worth a copy or an explicit comment at the pair boundary.-aphelp text: "omit together with--ceiling" reads as "omit both", which is precisely the case that errors. "omit when passing--ceiling" is what is meant.
On the --de-real / --de-pred comment above
--de-predin ceiling-only mode is incoherent (there is no prediction) — erroring is fair.--de-realis a reasonable thing to try, but the ceiling must compute DE on its own halves, so it cannot be reused. That reads as a warning rather than a hard failure — erroring turns a harmless no-op into a broken command.
Verified on the merge with current main
- ceiling-only run: exit 0, writes only the two ceiling CSVs.
--celltype-colpath: exit 0, per-celltype prefixed outputs.- ceiling values byte-identical across all 25 columns to the with-prediction run — the property the PR rests on.
- all four CI gates pass on the merged tree.
Minor
- Ceiling-only writes no
real_de.csv/pred_de.csv(correct — the self-split DE is in-memory), but that differs from the normal run. One line in the help text or PR body would save a question later.
…tyle usage error Tests for the new mode, which the suite did not exercise at all: - ceiling-only shape: main de_comparison skipped, pred side aliases real, compute() refuses instead of silently scoring real against itself - the ceiling is unchanged by supplying a prediction (same seed, same values) - the property the mode rests on - both precomputed-DE warnings fire - the CLI usage error exits 2 Precomputed de_pred/de_real cannot be reused in ceiling-only mode: the main comparison is skipped and the ceiling computes DE on its own disjoint halves. Both are now warned about rather than silently dropped, in MetricsEvaluator so the CLI and the programmatic API are covered by one check. A warning rather than an error, so a stray argument does not break an otherwise valid run. Omitting --adata-pred without --ceiling is a usage error, so exit 2 with a message on stderr the way argparse does, instead of surfacing a ValueError traceback. Record the pred-placeholder invariant: `pair.real is pair.pred` in ceiling-only mode - an alias, not a copy, since copying a matrix that is never scored would double peak memory. Safe only because compute() is blocked and compute_ceiling() derives fresh copies of both halves from .real, so an in-place write to .pred would corrupt .real. Help text: "omit when passing --ceiling" (omitting both is the case that errors), and note that ceiling-only writes no results.csv and no DE tables.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'ceiling-only' mode to the evaluation pipeline, allowing users to compute the real-data ceiling without providing a predicted AnnData object (adata_pred). The CLI has been updated to make --adata-pred optional when --ceiling is specified, and it now exits gracefully with a usage error if a prediction is omitted without enabling the ceiling. In the MetricsEvaluator, calling compute() is disabled in ceiling-only mode, and warnings are issued if precomputed differential expression tables are supplied. To support this mode efficiently, the evaluator aliases the real data as a placeholder for the prediction to avoid unnecessary memory overhead. Comprehensive unit tests have been added to verify these behaviors, CLI constraints, and output consistency. There are no review comments, so no feedback is provided.
There was a problem hiding this comment.
Pull request overview
This PR updates cell-eval run and MetricsEvaluator to support a ceiling-only workflow where the data ceiling is computed from the real AnnData alone, without requiring a prediction. This aligns the CLI and programmatic API with the fact that compute_ceiling() self-splits the real data and does not use the prediction.
Changes:
- Make
--adata-predoptional forcell-eval runwhen--ceilingis provided; otherwise treat it as a usage error. - Add a
ceiling_onlymode toMetricsEvaluator(adata_pred=None), skipping the main DE comparison and blockingcompute()with a clear error directing users tocompute_ceiling(). - Add tests covering ceiling-only evaluator invariants, CLI behavior, and warnings for unused precomputed DE inputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/cell_eval/_evaluator.py |
Adds ceiling-only mode (adata_pred=None), blocks compute(), and mirrors real→pred in _build_anndata_pair for placeholder pairing. |
src/cell_eval/_cli/_run.py |
Makes --adata-pred optional and adds ceiling-only CLI flow control (exit 2 when missing pred without --ceiling). |
tests/test_ceiling.py |
Adds ceiling-only mode tests, including CLI rejection and warning behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cell-eval runpreviously required--adata-pred. The data ceiling is estimated from the real data alone (disjoint self-split), so a prediction is unnecessary when only the ceiling is wanted.--adata-predis now optional; omitting it (together with--ceiling) computes only the real-data ceiling. Omitting it without--ceilingerrors.MetricsEvaluatoracceptsadata_pred=None(ceiling-only mode): skips the mainde_comparison, andcompute()raises directing tocompute_ceiling()._build_anndata_pairmirrors real into pred when no prediction is given (the placeholder is never scored; the ceiling reads only.real).