Restore the Diameter parameter in the Cellpose-SAM worker

[arjunrajlab]

Summary

a3e4524 (shipped in #152) removed the Diameter field from the cellposesam inference worker. The stated reason was that "Cellpose-SAM trains and evaluates at native resolution" — but that conflates two different cellpose 4.x parameters. Inference does still accept a diameter, so this restores the field.

The evidence

Verified against the pinned cellpose==4.2.1.1 sdist and the deeptile source:

Parameter	Status in v4.2.1.1
CellposeModel(diam_mean=...) — constructor	Genuinely deprecated: "not used in v4.0.1+. Ignoring this argument..."
CellposeModel.eval(diameter=...) — eval-time	Still fully honoured: if diameter is not None and diameter > 0: rescale = 30. / diameter

Our value reaches it: deeptile's cellpose_segmentation splats eval_parameters straight into model.eval(tile, **eval_parameters).

Coordinates are safe: with resample=True (cellpose's default, still live and undeprecated in 4.2.1.1) the flows are resized back to the original tile size, so polygons come out in native tile space regardless of any rescale.

Why the default is 30

The default is 30, cellpose's identity value — not a guess, and not an out-of-band 0 sentinel.

The 30 in rescale = 30. / diameter is a hardcoded literal inside CellposeModel.eval(), not a per-model diam_mean (v4 ignores that entirely). So Diameter = 30 gives rescale = 1.0 — precisely what diameter=None gives — and every downstream branch keys off rescale != 1.0 rather than off the diameter. The two are bit-for-bit equivalent, so the default run is byte-identical to today's behavior.

That matters beyond tidiness: because "off" is now a legitimate in-range value, the field can carry a meaningful minimum of 10. With 0-as-off it could not.

Behavior

Stored values are normalized by parse_diameter(), then the omission is keyed off the resulting rescale, so anything cellpose would treat as identity is dropped rather than passed through to no effect:

Stored value	Used	eval_parameters	Rescale	Warns?
absent / null / ""	30	{}	1.00	—
30 (default)	30	{}	1.00	—
"60" (numeric string)	60	{'diameter': 60.0}	0.50	yes
0 or negative	as given	{}	1.00	—
10 (old default)	10	{'diameter': 10.0}	3.00	yes
"abc", [30], True	—	sendError + raise	—	—

• Applies uniformly to base and custom models. Pre-removal it was applied to custom models only, and silently ignored for the base checkpoints.
• Out-of-range values are honored, not clamped. min/max are UI hints; a saved config can carry anything. Silently substituting a different diameter would change the segmentation, so the worker passes it through and warns.
• Warns on any active rescale, not just a large one — see below.

Code review round

A high-effort review found five issues; four were real and are fixed in ffb6119:

1. float('') crash (medium). compute() guarded only None, but '' is a documented "unset" shape for saved interface values in this repo — annotation_tools.get_selected_channels treats None/''/{} alike. A config whose Diameter had been cleared crashed with a bare traceback before any sendError. Parsing moved into parse_diameter(), which resolves unset shapes to the identity and raises ValueError on anything non-numeric so compute() can sendError + raise. Booleans are rejected explicitly, since bool is an int subclass and float(True) would quietly mean a 30x upscale.

2. Warning missed the case it existed for (medium). The warning was gated on tile_size * rescale > 2048. A pre-July-2026 config carries Diameter: 10 (3x upscale); at Tile Size 512 that is 1536 px — under the threshold, so the segmentation changed with no warning at all. It now fires on any rescale != 1.0.

3. Tile-size-relative threshold (low). Tile Size can be 0, and 0 * rescale never trips any threshold. Fixed by the same change; the GPU-memory caveat is now appended to the warning rather than gating it.

4. Incomplete test assertion (low). test_default_diameter_is_within_the_offered_range asserted only MIN <= DEFAULT, so raising DEFAULT to 500 would have kept it green. Now bounds both ends.

5. Doc citations (low). Hardcoded upstream line numbers rot across cellpose versions; replaced with symbol names.

One finding I did not apply. The reviewer also called the resample=True note stale, on the grounds that resample is deprecated in v4 and the resize-back is now unconditional. Verified against the pinned cellpose==4.2.1.1: resample is a live parameter defaulting to True with no deprecation warning, and the resize back is gated on if resample:. The note is accurate and stands, now qualified with the version it was checked against.

Codex review round (e8132ba)

Codex found two issues, both real and both fixed. The first was serious and mine.

P1 — NameError on every compute request. My previous commit rewrote the Diameter warning using a text-range replacement whose end anchor was the build_cellpose_parameters( call. That range also swallowed the block in between: the preview-client construction, models_dir = MODELS_DIR, and the custom-model download. models_dir was left undefined at its only use, so every valid inference request would have raised NameError before the model was built, and custom Girder models would no longer download. Restored verbatim; the diff against feb696b is now limited to the intended changes.

This had no test coverage, which is the more important finding. entrypoint.py cannot be imported in the lightweight venv (cellpose, deeptile, annotation_client), so nothing here catches a NameError until the worker runs on a GPU host against real data. Added tests/test_entrypoint_names.py: it walks the AST and asserts every name loaded in a function is bound somewhere — local, argument, comprehension or loop target, import, module-level name, or builtin. Verified it fails with exactly {'compute': ['models_dir']} against the buggy revision and passes against the fix, so it is a real regression test rather than a tautology.

P2 — non-finite diameters. float() accepts "inf", "nan", and overflowing literals like "1e309". Neither is safe to forward, and they fail in opposite directions:

• +inf passes cellpose's diameter > 0 guard and yields rescale = 30/inf = 0.0 — a degenerate zero-scale resize.
• NaN fails that guard, so cellpose would not rescale at all, while this worker's own 30/nan != 1.0 check would report that it had.

parse_diameter() now rejects non-finite values via math.isfinite(), and diameter_rescale() treats them as 1.0 as defence in depth so the two can never disagree.

Final suites:

workers/annotations/cellposesam/tests    41 passed
workers/annotations/cellposesam_train    17 passed
annotation_utilities                     89 passed
worker_client                            21 passed

Compatibility note for old configs

Tool configs saved before July 2026 still hold the old Diameter: 10. Under the pre-removal code that was ignored for base checkpoints; with this change it rescales by 3x on every model — and it now sits exactly at the interface minimum, so the UI looks normal while the image is upscaled. This is why the warning is no longer gated on tile size. Called out in CELLPOSESAM.md; those configs should have their Diameter re-checked or set to 30.

Tests

Test-first across three red→green rounds. 25 new cases in tests/test_models_config.py: the identity values (absent / null / '' / 0 / negative / 30), that 29 and 31 still pass through so only the exact identity is dropped, numeric strings, non-numeric and boolean rejection, no-clamping, custom-model parity, that the checkpoint choice is untouched by the diameter, and that diameter_rescale() matches cellpose's 30/diameter at the default, the minimum, and the old default of 10.

workers/annotations/cellposesam/tests    36 passed
workers/annotations/cellposesam_train    17 passed
annotation_utilities                     89 passed
worker_client                            21 passed

Docker build / GPU inference was not run in this environment.

Docs

CELLPOSESAM.md gets the parameter row back plus a "Diameter and Rescaling" section: why 30 is the default and identity, the diam_mean vs. eval(diameter=) distinction that caused the original removal, coordinate safety, warning behavior and the GPU-memory caveat, the parse_diameter() table for out-of-range and malformed values, and the old-config warning.

cellposesam_train's "no diameter needed" statements are correct and left alone — train_seg genuinely defaults to rescale=False. Training and inference differ here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UT5Ckq9RHyhbytJEMg3Wzp

[arjunrajlab]

@codex review

Restores the Diameter parameter to the Cellpose-SAM inference worker, which a3e4524 removed on the mistaken premise that Cellpose-SAM ignores it. Verified against the pinned cellpose==4.2.1.1: only the constructor argument diam_mean is deprecated in v4; the eval-time diameter is still honoured and deeptile forwards our eval_parameters straight into model.eval().

Points worth your attention:

1. The identity claim. The default is 30 because rescale = 30. / diameter uses a hardcoded 30 inside CellposeModel.eval(), making diameter=30 equivalent to diameter=None. build_cellpose_parameters() omits diameter from the eval call whenever the resulting rescale is 1.0. If that equivalence does not hold in some path I missed, the "default run is byte-identical" claim breaks.

2. parse_diameter() in models_config.py — normalizes stored config values. Unset shapes (None/''/whitespace) resolve to the identity; out-of-range numbers are honoured rather than clamped; non-numeric values raise ValueError which compute() turns into sendError + raise. Booleans are rejected because bool is an int subclass. Interested in any input shape this still mishandles.

3. Backward compatibility. Configs saved before July 2026 hold Diameter: 10 — a 3x upscale that the pre-removal code applied to custom models only and ignored for base checkpoints. The worker now warns on any rescale != 1.0 rather than gating on tile size, specifically because gating stayed silent for those configs at small tile sizes. Whether honouring the stored value (vs. migrating it) is the right call is a judgement I would like checked.

An earlier review round is already folded in (ffb6119); its findings and the one I rejected are summarised in the PR description.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffb6119143

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Restore the model-directory setup before parameter construction

Every valid Cellpose-SAM compute request now reaches this call with models_dir undefined because the preceding initialization and custom-model download block was deleted, so inference raises NameError before cellpose_segmentation is constructed. Restore that setup; it is also required to download uncached custom Girder models.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject non-finite diameter values

When a saved configuration or direct API request supplies values such as "Infinity", "1e309", or "NaN", float() accepts them instead of taking the documented malformed-value error path. Positive infinity produces a zero rescale that is forwarded to Cellpose and leads to invalid zero-scale image resizing, while NaN makes this worker report a rescale even though Cellpose's diameter > 0 guard treats it as inactive; reject non-finite results with math.isfinite().

Useful? React with 👍 / 👎.

[arjunrajlab]

Both Codex findings were real and are fixed in e8132ba. Thanks — the P1 was a serious one.

P1 (models_dir undefined): correct, and worse than "inference raises NameError" — the deleted range also took the preview-client construction and the custom-model download with it, so custom Girder models would not have downloaded either. My previous commit rewrote the warning block with a text-range replacement whose end anchor was the build_cellpose_parameters( call, and the range swallowed everything in between. Restored verbatim; the diff against feb696b is now limited to the intended changes.

The gap that let it through matters more than the bug. entrypoint.py cannot be imported in the lightweight test venv (cellpose, deeptile, annotation_client), so no test here would catch a NameError until the worker ran on a GPU host. Added tests/test_entrypoint_names.py, which walks the AST and asserts every name loaded in a function is bound somewhere — local, argument, comprehension/loop target, import, module-level name, or builtin. I confirmed it fails with exactly {'compute': ['models_dir']} against the buggy revision and passes against the fix, so it is a genuine regression test.

P2 (non-finite values): also correct, and the asymmetry you described is exactly right — +inf passes cellpose's diameter > 0 guard and produces a zero-scale resize, while NaN fails it, so cellpose would not rescale at all even though this worker's own 30/nan != 1.0 check would claim it had. parse_diameter() now rejects non-finite values with math.isfinite(), and diameter_rescale() treats them as 1.0 as defence in depth so the two can never disagree. Covered for inf, -inf, nan, and the overflowing '1e309' literal.

All suites green: 41 cellposesam, 17 cellposesam_train, 89 annotation_utilities, 21 worker_client. Docker build and GPU inference still have not been run in this environment.