Converge the dense read path onto the shared WSI reader#222
Merged
Conversation
Drive iter_regions_dense from an hs2p TilingResult and read through the shared batched WSIRegionReader (cuCIM read_regions) instead of the ad-hoc (coordinates, requested_spacing_um, wsi) contract + per-region read_region_at_spacing loop. The TilingResult already resolved spacing->level at tiling time (read_level / read_tile_size_px / requested_tile_size_px via plan_spacing_read), so the reader no longer re-plans per read: it reads read_tile_size_px at read_level and area-resizes to requested_tile_size_px when they differ, reusing hs2p resize_array(..., "area") -- the identical op read_region_at_spacing performed, so the read pixels are unchanged. - WSIRegionReader gains resize_to_px / interpolation (default off, so the hierarchical unfold path is unaffected); the dense reader passes requested_tile_size_px + "area". - iter_regions_dense drops wsi/coordinates/requested_spacing_um/target_size and takes a TilingResult (+ backend / num_workers knobs); all encode-side params are unchanged. Breaking dense-API change (hard replacement). - Backend resolved the way the pooled path does (resolve_slide_backend). - get_dense_transform (normalization-only) left untouched. - Tests rewritten onto the new offline seam (monkeypatch tile_reader._open_wsi_backend), preserving coordinate order, eager validation, empty input, output_dtype, bf16 rejection, streaming, and batch-invariance; adds the area-resize and serial-backend cases.
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
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.
Closes #216
What this does
Converges the dense extraction read path onto the same batched WSI reader the pooled path uses, driven by an hs2p
TilingResult, and retiresiter_regions_dense's ad-hoc(coordinates, requested_spacing_um, wsi)contract.Before this change the dense path had drifted from the pooled path: it took raw coords + a scalar spacing + an injectable
wsiand read one region at a time through the high-level per-callwsi.read_region_at_spacing, instead of keeping theTilingResultand reading through the low-level batchedWSIRegionReader(cuCIMread_regions(num_workers=…)). That drift was the root cause of the "overlap reads with the forward" problem; convergence removes the need for any bespoke prefetcher (supersedes #220).Changes
iter_regions_denseis now driven by aTilingResult. It dropswsi,coordinates,requested_spacing_um,target_size,toleranceand takestiling_result(carryingtiles.x/y,read_level,read_tile_size_px,requested_tile_size_px,image_path,backend) plus two reader knobs (backendoverride,num_workers). All encode-side params are unchanged (model, device, window_size, overlap, feature_kind, attention_*, batch_size, precision, output_dtype, pad_mode, image_pad_value, dense_transform).WSIRegionReader(batched cuCIMread_regions; serialread_regionfor non-cucim backends). The bespoke per-regionread_region_at_spacingloop and thetorch.stack([_read_padded(loc) …])serial read are gone. No standaloneThreadPoolExecutor.WSIRegionReadergainsresize_to_px/interpolation(defaultresize_to_px=None→ no resize, so the hierarchical unfold path is untouched). The dense reader passesrequested_tile_size_px+"area"; the resize is applied iffread_tile_size_px != requested_tile_size_px, reusing hs2p's ownresize_array(..., interpolation="area").areais preserved (not switched to bilinear — a real, non-tolerance pixel change; seedocs/adr/0002).resolve_slide_backend): an explicitbackendwins, else it falls back to theTilingResult's own backend ("auto" resolution).get_dense_transform(normalization-only) is untouched — the encoder still receives the fullrequested_tile_size_pxtile;dynamic_img_sizehandles it. No model-input resize was added to the dense path.docs/api.rst) and module docstrings updated to the newTilingResult-driven contract.Output preservation (ADR-0002)
The converged dense pixels are identical to the pre-convergence output:
TilingResult.read_level/read_tile_size_pxare set directly fromplan_spacing_read(requested_spacing_um=…, target_size_px=(requested_tile_size_px, requested_tile_size_px), tolerance=…)at tiling time (hs2p/tiling/generate.py). That is the exact callread_region_at_spacingruns internally, so the level + read size are the same ones the old dense path would have picked per read.read_tile_size_pxatread_levelthen area-resizes torequested_tile_size_pxvia the identicalresize_array(..., "area")the old path used (when levels match exactly it is a no-op, lossless).get_dense_transformviaImage.fromarray, unchanged.Same level + same
resize_array→ identical read pixels → dense grids are preserved within the encode-side ADR-0002 float tolerance (per-grid cosine ≥ 1 − 1e-4). The rewritten test suite pins byte-identity of the streamed grids against a hand-rolled read → (area-resize) → transform → pad → encode reference, for both feature kinds and the whole + sliding-window paths, including the resized (read != requested) case.Breaking change / downstream
This is a breaking dense-API change (hard replacement, no back-compat shim — the old signature is gone). The required downstream update is soma#279:
soma/dense_slide_extraction.pymust stop unpacking theTilingResultinto coords + spacing and pass it through instead. soma stays on the prior slide2vec until updated. Note for release notes.Tests
tests/test_dense_regions.pyrewritten onto the new offline seam — monkeypatchslide2vec.data.tile_reader._open_wsi_backendto return a fake backend serving canned regions viaread_regions/read_region. Preserves the prior behavioural coverage (coordinate order, eager validation before any read, empty input,output_dtype, bfloat16 rejection, one-batch-at-a-time streaming) and adds: the area-resize case (read_tile_size_px != requested_tile_size_px), a serial-backend case (non-cucimread_region), and an explicit batch-invariance check (composition irrelevant, onlyBmatters).Full suite (CPU, no GPU):
pytest -q -m "not heavy" --no-cov tests→ 467 passed, 25 deselected. (--no-covused because the repo's default--covsqlite flakes on this network filesystem; no coverage-relevant change.)Notes for the reviewer
TilingResult.resizeddoes not exist in the pinned hs2p (4.2.0) — the field is only onSam2Thumbnail. The operative resize condition isread_tile_size_px != requested_tile_size_px, which is what the issue describes and what the pooled tar path (hs2p/tiling/tar.py) uses. Implemented against that condition.