diff --git a/docs/design/spatial-tables/00-table-element-association.md b/docs/design/spatial-tables/00-table-element-association.md new file mode 100644 index 000000000..e09794a54 --- /dev/null +++ b/docs/design/spatial-tables/00-table-element-association.md @@ -0,0 +1,202 @@ +# 00 — First-class table ↔ spatial-element association (the core deliverable) + +> **This is the headline user-facing feature.** The other docs (01–05) are enablers or +> adjacent work. The tangible win: a spatial **element** (shapes / labels) that is annotated by a +> **table** gets a proper, first-class link to that table's rows — so the geometry is **colored, +> filtered, highlighted, and tooltipped by table columns**, and filtering flows both ways between +> the geometry and every other chart on that table. + +## What "first-class" means (the proven reference: the points path) + +MDV already has a first-class table↔geometry association — for the **points** representation. The +SpatialData region scatter renders a table's rows as deck.gl points and is first-class because +**three concerns share one per-row index space, all rooted on the `DataStore`** +([src/react/scatter_state.ts](../../../src/react/scatter_state.ts)): + +- **Shared filter mask.** `data` fed to the layer is `ownerVisibleRows` — a `Uint32Array` of + surviving row indices from `useOwnedFilteredIndices()` + ([hooks.ts:520](../../../src/react/hooks.ts)). Only passing rows draw; brushing *any* chart + updates `ds.filterArray` and the geometry redraws. Lasso on the spatial view feeds back via + `spatial_context.tsx` → `rangeDimension.filterPoly` → `ds.filterArray`. +- **Shared color pipeline.** `getFillColor: colorBy`, where + `colorBy = chart.getColorFunction(col, true)` returns `(rowIndex) => [r,g,b]` + ([BaseChart.ts:683](../../../src/charts/BaseChart.ts) → [DataStore.js:1524](../../../src/datastore/DataStore.js)). + `updateTriggers.getFillColor: colorBy` recolors by function-reference identity. A legend is + produced as a side effect. +- **Shared highlight.** Deck picking `onClick → dataHighlighted(data[index])` + ([selectionHooks.ts:133](../../../src/react/selectionHooks.ts)); highlight rings render from + `useHighlightedIndices()`. + +**The goal is to give shapes/labels geometry the exact same three properties**, keyed on the same +`DataStore` row-index space — so a shape and a point representing the same cell filter, colour, and +select together. + +## The two halves that must meet + +### MDV side — per **row** (already exists) +`ds.filterArray` (shared filter) · `chart.getColorFunction(col, true)` (row → colour) · +`dataHighlighted(rowIndex)` (shared highlight) · tooltip via `config.tooltip.column`. All indexed +by **DataStore row**. + +### Library side — per **feature** (already exists, in SpatialData.ts) +The association layer lives in +`SpatialData.ts/packages/core/src/tableAssociations.ts` and is element-agnostic: + +- `loadAssociatedTableFeatureRows({spatialData, kind, key, extraColumnNames})` resolves the + annotating table via `getAssociatedTable` (matching the element key against the table's `region` + array), then builds `rowIndexByFeatureId: Map`, filtered by + the `region_key` column. Critically `TableElement.loadObsIndex()` returns the **`instance_key`** + column as row ids (not `obs.index`) — matching Python spatialdata's join-by-instance semantics. +- `loadFeatureRowIndexByFeatureIndex({..., featureIds})` → **`Int32Array`** answers "rendered + feature *i* → table row" (`-1` = unmatched). +- For shapes, `ShapesElement.loadRenderData()` returns `ShapesRenderData` with + `featureIds: string[]` and **`rowIndexByFeatureIndex: Int32Array` pre-filled**. Geometry loads + from `shapes//shapes.parquet` (parquet-wasm → arrow, WKB-decoded) in parquet row order. +- The library's shapes deck layers (`PolygonLayer` / `ScatterplotLayer`, **not** `GeoJsonLayer`) + already carry `{featureId, featureIndex, rowIndex}` on every datum, expose a per-feature + colouring surface `featureState.fillColorByFeatureId` / `hiddenFeatureIds` / `fadedFeatureIds`, + and emit pick events that include `rowIndex` + (`SpatialData.ts/packages/layers/src/shapesLayer.ts`). + +### The bridge +`rowIndexByFeatureIndex[i]` maps rendered feature *i* → DataStore row. Invert it (via `featureIds`) +to go row → featureId when handing colours to the library's `featureState` (which is +**featureId-keyed, not row-keyed**). + +### Alignment guarantees (from the library) +- **Instance-key join** is the primary contract: feature id (shape GeoDataFrame index / label + pixel value) ↔ the table row whose `instance_key` equals it, restricted by `region_key`. +- **Positional fallback (shapes only)** fires *only* when feature ids are exactly `"0".."n-1"` (a + synthesized parquet RangeIndex) and row counts match — then feature *i* ↔ the *i*-th filtered + table row. Fragile; assumes parquet order == filtered-table order. +- Unmatched features get `-1` and are surfaced explicitly, not guessed. + +## Why nothing connects today + +The two halves exist but are not wired: + +- `table_association.ts` ([src/react/spatialdata/table_association.ts](../../../src/react/spatialdata/table_association.ts)) + is a **type stub** (`none | ambiguous | resolved`) with **no resolver** — `NO_TABLE_ASSOCIATION` + is hardcoded at both panel call sites. +- The `fillColorByColumn` and `tooltipFields` UI in + [ShapesLayerPanel.tsx:138](../../../src/react/components/spatialLayers/ShapesLayerPanel.tsx) is + **commented out** ("not working yet"); the `LabelsLayerPanel` is **disabled**. +- Shapes render with a **static fill**, ignore `ds.filterArray`, and are not picked into the + DataStore. Their tooltip is library-driven, not MDV column-driven. +- MDV supplies **no per-feature accessors** to the library's shapes layer today — only static + `fillColor` / `strokeColor` props via the render-stack `LayerConfig`. + +## The Python contract gap (why this pulls toward the JS-read path) + +The association joins on **`instance_key`** (or positional row order). The current `convert-spatial` +path **destroys both**: + +- `convert-spatial` reads `instance_key` via `get_table_keys` but **discards it** — `_instance_key` + at [conversion.py:373](../../../python/mdvtools/spatial/conversion.py) and `:639`. The only + per-row id that survives is `mdv_cell_id`, derived from the **merge-suffixed** obs index + ([conversions.py:242](../../../python/mdvtools/conversions.py)). +- The single-`anndata.concat` merge **row-stacks all tables** ([02-convert-spatial-tables.md](02-convert-spatial-tables.md)), + scrambling row order — so even positional alignment can't be trusted. + +**Consequence:** the clean association resolves against the **untouched zarr store**, where +SpatialData.ts's association layer already computes the feature→row map. So the headline feature is +most naturally delivered on the **JS-read path** — a spatial chart backed by the SpatialData object +directly ([01-js-dataloader.md](01-js-dataloader.md)) plus the shared store cache +([03-shared-spatial-contexts.md](03-shared-spatial-contexts.md)). The alternative is to fix the +converter (theme 2) to **preserve `instance_key` and `region_key` as columns and stop scrambling +row order** (one-datasource-per-table / merge-by-region) so the h5 path can join too. + +> This is the single most important strategic conclusion of the session: **the core deliverable +> and the JS DataLoader / converter rework are the same project, not separate ones.** You cannot +> have first-class association on top of the current lossy h5 flattening. + +## Rendering options + +Both need the **prerequisite**: an association resolver (fill in `table_association.ts`) that maps +an element (`kind`, `elementKey`, coordinate system) → the MDV `DataStore` for its table, and +builds the feature→row `Int32Array` (via `ShapesElement.loadRenderData()` / +`loadFeatureRowIndexByFeatureIndex`). + +### Option A — drive the library's `featureState` (reuse library geometry *and* rendering) +Compute, from MDV per-row data, the featureId-keyed inputs the library layer already consumes: +- `fillColorByFeatureId[featureId] = colorBy(rowIndexByFeatureIndex[i])` using + `chart.getColorFunction(col, true)` — **the same colour pipeline as points**. +- `hiddenFeatureIds` / `fadedFeatureIds` from `ds.filterArray` (a feature is hidden/greyed iff its + row is filtered out) — **the same filter mask as points**. +- wire the library's shape pick event (`rowIndex`) → `dataHighlighted` and `config.tooltip.column`. + +*Pro:* reuses the library's geometry loading *and* deck layers; least new rendering code. +*Con / must-verify:* requires `@spatialdata/vis`'s renderer (`useSpatialCanvasRendererFromLayerInputs`) +to accept per-layer `featureState` (or equivalent colour/hidden inputs) through the render-stack +`LayerConfig` / renderer inputs. The `@spatialdata/*@0.2.5` packages are **not installed in this +worktree** — confirm this injection seam against installed `node_modules` before committing to A. + +### Option B — MDV host deck layer using library-loaded geometry (recommended for parity) +Call `ShapesElement.loadRenderData()` to get geometry + `rowIndexByFeatureIndex` (so **no WKB / +parquet reimplementation**), then build an **MDV-owned** `PolygonLayer` / `ScatterplotLayer` as a +**host overlay** — MDV already injects host deck layers (`deck:scatter`, gates, selection) into the +spatial canvas via `createMdvHostLayerResolver` +([render_stack_adapter.ts](../../../src/react/spatialdata/render_stack_adapter.ts), +[host_overlay_ids.ts](../../../src/react/spatialdata/host_overlay_ids.ts)). The overlay mirrors the +points path verbatim: +- `data` = features whose mapped row is in `ownerVisibleRows` (shared filter), +- `getFillColor: (f) => colorBy(rowIndexByFeatureIndex[f.featureIndex])` with + `updateTriggers.getFillColor: colorBy` (shared colour), +- `pickable`, `onClick → dataHighlighted(row)` + highlight stroke from `useHighlightedIndices()` + (shared highlight), +- tooltip via `config.tooltip.column` over the mapped row. + +*Pro:* genuinely first-class — one `filterArray`, one `getColorFunction`, one `dataHighlighted` +shared with points; no dependence on the library's (unverified) colour-injection surface. +*Con:* a new host-layer category (generalize the fixed `deck:*` id enum); MDV owns a second shapes +renderer alongside the library's. + +**Recommendation:** prototype **A** first (cheapest if the seam exists); fall back to **B** for +guaranteed parity. Either way the colour machinery to reuse is `chart.getColorFunction(col, true)` +→ `(row) => [r,g,b]`, wired by function reference — the exact seam the points layer uses. + +### Labels are harder (raster-only) +Labels have **no vector geometry** — only a segmentation raster; "features" are integer pixel +values (label ids == instance_key), resolved to rows only at pick time +(`tooltipRowIndexByFeatureId.get(String(labelId))`, instance-key join, no positional fallback). So: +- **Colour by table column** = build an `instanceKey → [r,g,b]` LUT from `getColorFunction` and have + the label layer recolour the raster by that map (check whether `@spatialdata/vis`'s labels layer + accepts a colour map; `useLayerData` has a labels path). +- **Filter** = a hidden-label mask (paint filtered rows to background/transparent). +- You **cannot enumerate all label features** without scanning the raster. +Do labels **after** shapes; treat colour-by-LUT as the MVP and filtering as a follow-up. + +## Bidirectional filtering + +- **Table → geometry:** `ds.filterArray` change → recompute hidden/visible features → redraw + (Option A `hiddenFeatureIds`, or Option B `data = ownerVisibleRows`). Free once the bridge exists. +- **Geometry → table:** lasso/box on the spatial view → point-in-polygon over shape + centroids (or the picked feature set) → `rangeDimension.filterPoly` → `ds.filterArray`, exactly + as the points path already does via `spatial_context.tsx`. + +## Phasing + +0. **Association resolver** — fill `table_association.ts`: map `elementKey` → the table's DataSource + (via region metadata), build the feature→row `Int32Array`. Everything else depends on this. +1. **Colour shapes by a table column** — reuse `getColorFunction`; drive Option A `featureState` or + Option B host layer. Re-enable the `fillColorByColumn` UI in `ShapesLayerPanel`. +2. **Filter shapes by the table** — `filterArray` → hidden/faded features; redraw. +3. **Pick + highlight + tooltip shapes** → `dataHighlighted`, `config.tooltip.column`. +4. **Geometry → table lasso selection** (bidirectional filtering). +5. **Labels**: colour-by-LUT, then hidden-label filtering. + +## Open questions / risks + +- **Which DataSource is "the table" for an element?** The resolver must map `elementKey` → region → + the MDV datasource name. Clean on the JS path (the store's `getAssociatedTable`); needs + converter-preserved region metadata on the h5 path. +- **The converter identity gap** (above) — decide JS-read vs converter-fix as the delivery route. +- **`@spatialdata/vis` `featureState` injection seam** — verify against installed packages + (worktree lacks them) before betting on Option A. +- **Multi-region tables**: the library's `getAssociatedTable` uses only the *first* match; a + broad "matched zero → use all rows" fallback can over-associate for shared tables. +- **Positional-alignment fragility** — safe only for `0..n-1` parquet indices with matching row + counts; another reason to prefer real `instance_key`s end to end. +- **Perf**: `loadFeatureRowIndexByFeatureIndex` re-scans the whole obs table per call; cache it in + the project-scoped store cache (theme 3) for interactive use. +- **Labels raster limits** — no feature enumeration; colour/filter via LUT + masking only. diff --git a/docs/design/spatial-tables/01-js-dataloader.md b/docs/design/spatial-tables/01-js-dataloader.md new file mode 100644 index 000000000..3f3f56714 --- /dev/null +++ b/docs/design/spatial-tables/01-js-dataloader.md @@ -0,0 +1,184 @@ +# 01 — A JS (spatialdata.js / anndata.js) DataLoader with h5 write routing + +> Read [README.md](README.md) first for the four load-bearing facts. This doc details the +> data-layer contract, what a zarr read loader must produce, and how it reconciles with the +> existing h5 write path. + +## The goal + +Add a `DataLoader` variant whose `.function` reads column data directly in the browser from a +SpatialData / AnnData **zarr** store (via `spatialdata.js` / `anndata.js`), instead of POSTing +to the Flask `/get_data` endpoint. Some columns remain **h5-backed and writable** — especially +anything the user edits — so the loader must know which columns to read from zarr and which to +defer to the server, and writes must continue to land in `datafile.h5`. + +## Current data layer (grounded) + +### The DataLoader contract + +`getDataLoader(isStaticFolder, datasources, views, url)` +([src/dataloaders/DataLoaderUtil.ts:85](../../../src/dataloaders/DataLoaderUtil.ts)) returns: + +```ts +type DataLoader = { + function: (columns: ColumnInfo[], dataSource: string, size: number) + => Promise>, // real contract + viewLoader: (view: string) => Promise, + rowDataLoader: (dataSource: string, index: string) => Promise, + binaryDataLoader:(dataSource: string, name: string) => Promise, +} +``` + +> The declared type of `.function` (`Promise`) is wrong/loose; the runtime +> contract is a **per-column list of `{field, data}`**. Worth fixing while here. + +Today there are two variants, selected by `isStatic` +([projectRuntime.ts:61](../../../src/modules/projectRuntime.ts)): +- **Server** (`isStatic=false`): `getArrayBufferDataLoader('{root}/get_data')` — one POST per + batch, returns a single concatenated buffer split by `processArrayBuffer` + ([DataLoaders.ts:197](../../../src/dataloaders/DataLoaders.ts)). +- **Static** (`isStatic=true`): `getLocalCompressedBinaryDataLoader` — per-column HTTP Range + requests against `{ds}.gz` with a `{ds}.json` byte-offset index + ([DataLoaders.ts:238](../../../src/dataloaders/DataLoaders.ts)). + +### The invariants a loader must honor + +`processArrayBuffer` ([DataLoaders.ts:106](../../../src/dataloaders/DataLoaders.ts)) is the +canonical byte→typed-array spec. Any loader must produce, per column, a `SharedArrayBuffer` +laid out as: + +| datatype | TypedArray | notes | +|---|---|---| +| `integer` / `int32` | `Int32Array` | length `size` | +| `double` | **`Float32Array`** | MDV stores double as **float32** — downcast float64 | +| `text` | `Uint8Array` | length `size`; values are **indexes into `column.values`** | +| `text16` | `Uint16Array` | length `size`; indexes into `values` | +| `unique` | `Uint8Array` | fixed width `stringLength`; length `size*stringLength` | +| `multitext` | `Uint16Array` | width `stringLength`, `65535` sentinel for empty | + +Plus: +- **Column-major**: one contiguous buffer per column, exactly `size` rows. `size` is fixed per + datasource from `datasources.json` ([DataStore.js:74](../../../src/datastore/DataStore.js)). +- **SharedArrayBuffer**, not `ArrayBuffer` — data is posted to workers without copying. (Requires + cross-origin isolation / COOP+COEP; verify headers if reading from a CDN.) +- **Categorical columns store integer indexes into a `values: string[]` array**, and `values` + must be present in metadata and **stable across read and write** or colors/filters break. +- **Sparse matrix columns** expand to a dense `Float32Array(size)` pre-filled with `NaN`. +- The **4-byte alignment ordering** (numeric columns first) only matters if you build a single + concatenated buffer. **Return one SAB per column and it is a non-issue** — recommended. + +### Where loaded bytes land + +`ChartManager._loadColumnData` ([ChartManager.js:1495](../../../src/charts/ChartManager.js)) +calls `this.dataLoader(columns, dataSource, size)`, then for each returned `{field, data}` calls +`dataStore.setColumnData(field, data)` ([DataStore.js:1262](../../../src/datastore/DataStore.js)), +which accepts a `SharedArrayBuffer` directly. `columnsWithData.push(field)` marks it loaded. +**None of this cares where the bytes came from.** + +### The write path (unchanged by any read loader) + +Edits: `getState()` → `_getUpdatedColumns` (materializes typed arrays back to JS arrays, +[ChartManager.js:1101](../../../src/charts/ChartManager.js)) → `state_saved` listener → +`POST /save_state` → `save_state` → `set_column_with_raw_data` +([mdvproject.py:849](../../../python/mdvtools/mdvproject.py)) → h5 `create_dataset`. The write +target is **always `datafile.h5`**. Writability is whole-project (`MDVProject.writable`, +[mdvproject.py:139](../../../python/mdvtools/mdvproject.py)); there is no server-side per-column +`editable` flag (the client sends one but the server ignores it). + +## What the JS loader must implement + +A new factory, e.g. `getZarrDataLoader(datasources, storeDescriptor)`, returning the `DataLoader` +shape. Its `.function`, given `(columns, dataSource, size)`: + +1. Resolve the datasource's zarr store (ideally from the **project-scoped store cache** — see + [03-shared-spatial-contexts.md](03-shared-spatial-contexts.md)). +2. For each requested column, read `size` values keyed by `column.field` (or + `subgroup`/`sgindex` for matrix-backed rows-as-columns). +3. Encode to the exact typed-array layout above, wrap in `SharedArrayBuffer`, return + `{field, data}[]`. +4. Provide `viewLoader` / `rowDataLoader` / `binaryDataLoader` — these can **delegate to the + existing server loaders**; only column data comes from zarr. + +### What the JS libraries give us (and don't) + +From the read of `anndata.js` and `SpatialData.ts` (both **read-only**, both on `zarrita`): + +- **Whole-column reads are cheap and the natural mode.** `SpatialData.ts` + `TableElement.loadObsColumns([name])` → `VAnnDataSource._loadColumn` + (`SpatialData.ts/packages/core/src/models/VAnnDataSource.ts:86`) returns plain JS + `TableColumnData` with **categoricals already decoded to `string[]`** and per-column promise + caching. This is the **most directly reusable** path — closest analogue to MDV `get_data`. +- `anndata.js` (`obs.get(name)` + `get(handle, [null])`) returns zarrita `Chunk`s; better for + **X-matrix** slicing (dense and sparse). Sparse densifies a whole **major-axis** slice — pick + CSR vs CSC to match the hot access direction (cell-wise vs gene-wise). +- **No arbitrary row-index gather** in either library. If `get_data` is asked for a filtered row + subset, load the whole column and index in JS, or drop to zarrita for chunk-aware gather. +- Column discovery: `TableElement.getObsColumnNames()` (sync, from the parsed tree). `anndata.js` + has no "list columns" method. +- **Version skew risk:** `anndata.js` pins `zarrita@0.5.1`; `SpatialData.ts` targets `0.7.x`. + Reconcile before sharing a bundle. `anndata.js` is `0.0.x` — expect API churn. +- MDV already depends on `@spatialdata/*@^0.2.5` and `zarrextra` (used today only for image + layers, not column data) — so the packages are on hand. + +### The one net-new client concept: `backing` + +The client has **no model today for "read from zarr, write to h5."** Introduce column metadata, +e.g. `backing: "h5" | "zarr"` (or `readSource`/`writeTarget`), threaded through: +- `datasources.json` schema (`DataSourceSchema`, validated in + [DataStore.js:29](../../../src/datastore/DataStore.js)), +- `DataColumn` type ([charts.d.ts:75](../../../src/charts/charts.d.ts)), +- `getColumnInfo` ([DataStore.js:1876](../../../src/datastore/DataStore.js)), +- `_getUpdatedColumns.getMd` ([ChartManager.js:1155](../../../src/charts/ChartManager.js)) so the + save payload can carry it if the server ever needs it. + +The loader consults `backing` to **split the column list**: zarr for `backing:"zarr"`, HTTP +`/get_data` for the rest; merge the `{field,data}[]` results. `_loadColumnData` passes the whole +list and just consumes the merged result, so the fan-out is internal to the loader. + +## Recommended design: a hybrid read loader + +- `.function` receives the full column list, partitions by `backing`, reads zarr columns via + `SpatialData.ts`'s `loadObsColumns` (whole-column), reads the rest via the existing `/get_data` + POST, encodes both to SABs, and merges. +- **Staleness rule:** a user edit writes the column to h5 and upserts its metadata into + `datasources.json`. The simplest rule: **once a column exists as an h5 dataset, prefer h5.** + `/get_configs` already returns live datasource metadata after a save, so an h5-shadowed column + can be flagged there and the loader routes it to `/get_data` on the next load. +- **Server changes are optional.** `/save_state`→`set_column_with_raw_data` already writes any + column to h5. You only touch the server if you want edits mirrored back into zarr, or want the + server to track the h5-shadow set for read routing. + +### Encoding pitfalls to plan for + +- **`double` → float32** downcast: watch precision on columns that round-trip zarr → edit → h5. +- **Categorical `values` ordering** must be identical on read and write; `anndata.js` / + `SpatialData.ts` eagerly decode codes→strings, but MDV wants **codes + a `values` dictionary** + — consider reading the `codes`/`categories` zarr arrays directly to preserve the compact form + rather than re-deriving `values` from decoded strings. +- **`unique` fixed width**: zarr strings are variable-length; compute `stringLength = max` and pad + to match `_convertColumn` / `processArrayBuffer`. +- **Sparse X**: MDV hands `DataStore` a dense `NaN`-filled `size`-length SAB even for sparse — + memory-heavy for large gene matrices; a zarr loader can stream but must still densify per column. + +## Phasing + +1. **Read-only proof of concept.** New `getZarrDataLoader`; branch in `getDataLoader` keyed on a + per-datasource `store` descriptor (stub type `ExperimentalZarrStore` exists at + [charts.d.ts:161](../../../src/charts/charts.d.ts)). All columns from zarr; no writes yet. + Validate numeric + categorical columns against a known project. +2. **Hybrid routing.** Add `backing` metadata; fan out zarr vs `/get_data`. Confirm an edited + column round-trips to h5 and reloads from h5. +3. **Store-cache integration.** Read from the project-scoped `SpatialData` cache (theme 3) so the + loader and the spatial charts share one opened store. +4. **Matrix / rows-as-columns.** Serve gene-expression subgroups from zarr X (choose CSR/CSC). + +## Open questions / risks + +- **Source of truth after an edit** (zarr vs h5) — the "prefer h5 if dataset exists" rule needs a + reliable signal; decide whether the server or the client owns the shadow set. +- **SharedArrayBuffer cross-origin isolation** when zarr is served from a different origin. +- **Row-index gather** absence — acceptable if `get_data` stays whole-column (it is today), but + revisit if async filtering (theme 5) wants to fetch only passing rows. +- **Version alignment** of zarrita across `anndata.js` / `SpatialData.ts` / MDV. +- **Greenfield**: `@spatialdata/*` is wired only to image rendering today — no prior art for the + column-data path in MDV. diff --git a/docs/design/spatial-tables/02-convert-spatial-tables.md b/docs/design/spatial-tables/02-convert-spatial-tables.md new file mode 100644 index 000000000..9f1bf7c08 --- /dev/null +++ b/docs/design/spatial-tables/02-convert-spatial-tables.md @@ -0,0 +1,145 @@ +# 02 — Flexible table handling in `convert-spatial` + +> How SpatialData `tables` become MDV datasources today, why the current single-concat merge +> makes a mess for heterogeneous tables, and the design space for fixing it. + +## The problem in one sentence + +The converter iterates **every table of every store into one flat list** and does **one +`anndata.concat`**, always producing **exactly one obs datasource + one var datasource** — so a +`grid`/bins table and a `cells` table get row-stacked into a single datasource with a zero-filled +union gene matrix, which is meaningless. + +## Current pipeline (grounded) + +CLI: `convert-spatial` at [cli.py:172](../../../python/mdvtools/cli.py), handler +`convert_spatial` at [cli.py:192](../../../python/mdvtools/cli.py) → builds +`SpatialDataConversionArgs` ([conversion.py:23](../../../python/mdvtools/spatial/conversion.py)) +→ `convert_spatialdata_to_mdv` ([conversion.py:968](../../../python/mdvtools/spatial/conversion.py)). + +> Two papercuts noticed in passing: the click positional args are declared +> `output_folder` then `spatialdata_path` but the handler signature is +> `(spatialdata_path, output_folder, …)` — click binds by name, so on the CLI you type +> `convert-spatial OUTPUT_FOLDER SPATIALDATA_PATH`. And `--point-transform` is exposed only on the +> argparse / report entrypoints, not the click CLI. + +The merge, `_concat_spatial_tables` +([conversion.py:885](../../../python/mdvtools/spatial/conversion.py)), called at +[conversion.py:1117](../../../python/mdvtools/spatial/conversion.py): + +```python +def _concat_spatial_tables(adata_objects): + return ad_concat(adata_objects, index_unique="_", join="outer", fill_value=0) +``` + +As pseudo-code: + +```text +adata_objects = [] +for store in discovered_stores: + for (table_name, adata) in read_zarr(store).tables.items(): # EVERY table, no grouping + resolve_regions_for_table(adata) # writes obs.x/y/spatial_region, uns.mdv + adata.obs["spatialdata_path"] = store_name + adata.obs["table_name"] = table_name # the ONLY per-table provenance + adata_objects.append(adata) # flat list, all shapes mixed +merged = anndata.concat(adata_objects, index_unique="_", join="outer", fill_value=0) +convert_scanpy_to_mdv(output, merged, ...) # -> ONE obs ds + ONE var ds +``` + +`convert_scanpy_to_mdv` ([conversions.py:118](../../../python/mdvtools/conversions.py)) then maps +`obs`→`cells`, `var`→`genes`, links them with `add_rows_as_columns_link`, and stores `X` as a +`rows_as_columns` subgroup. + +### Element → MDV construct (today) + +| SpatialData element | MDV construct | +|---|---| +| `tables` obs | rows of the **single** `cells` datasource | +| `tables` var | rows of the **single** `genes` datasource | +| `tables` X / layers | `rows_as_columns` subgroups (gene scores) on `cells` | +| `tables` obsm (spatial, umap) | `x`/`y` + dim-reduction columns | +| `shapes` (annotated region) | GeoJSON `images/.geo.json` + region entry; coords → obs x/y | +| `labels` | coordinate transform / region only; **no datasource** | +| `points` | **not converted** | +| `images` | `viv_image` per region | +| region / region_key / instance_key | consumed to compute regions; only `spatial_region` text column survives | + +## Why it makes a mess + +SpatialData's data model (`TableModel`, `get_table_keys` → +`(region, region_key, instance_key)`) says **each table annotates a distinct element / entity +type**. The merge ignores this: + +- **Rows are stacked across entity types.** `n_obs_merged = Σ n_obs`. A grid table (raster + lattice, e.g. Visium HD 2µm bins) and a cells table (segmented cells) end up as one datasource + whose rows are a mix of grid squares and cells. Row count is meaningless; per-cell analysis is + polluted by grid rows and vice-versa. +- **The gene axis is force-unioned.** `join="outer"` + `fill_value=0` across unrelated feature + spaces (e.g. a protein panel vs a transcriptome) produces a huge block that is `0` everywhere a + feature doesn't belong — statistically and visually misleading. (Outer join is deliberate: + inner join would collapse disjoint gene sets to zero, comment at conversion.py:887.) +- **Row→element is only a text column.** After merge there is one obs datasource but multiple + regions; a row relates to its element solely via the soft `obs["spatial_region"]` string, not a + structural link. +- **Per-table embeddings leak into shared columns.** `compute_x_umap` UMAP/leiden are explicitly + *not* comparable across tables, yet land in shared `X_umap`/`leiden` columns; leiden categories + are per-table-prefixed as a workaround (`_prefix_table_leiden_categories`, conversion.py:938). + +## Design space + +The lever: replace the single flat list + single `_concat_spatial_tables` +([conversion.py:1036–1131](../../../python/mdvtools/spatial/conversion.py)) with a **grouping +step** — `group_tables(adata_objects) -> {group_name: [adata]}` — then loop groups, producing one +`(obs, var)` datasource pair per group via `convert_scanpy_to_mdv` (which already accepts custom +datasource names). A new field on `SpatialDataConversionArgs` selects the policy. + +| Option | How | Pros | Cons | Effort | +|---|---|---|---|---| +| **1. One datasource per table** | each `(store, table)` → own obs/var pair, names from table name (`_allocate_table_prefix` exists) | never mixes entities; simplest correct default; native gene sets, no zero-fill; embeddings stay per-table | many datasources for big batches; cross-table compare needs explicit links; default-view template assumes single `cells`/`genes` | Moderate | +| **2. Merge by region / annotated element** | group by `get_table_keys` `region` (or `instance_key` space); concat within group | semantically principled; `cells` across stores merge, `cells`+`grid` stay separate; reuses region resolution | needs stable "same element type" identity across stores; no-attrs tables need a fallback bucket | Moderate | +| **3. Merge by compatible shape (auto)** | concat only tables with same `instance_key` semantics and/or var overlap above a threshold + same kind | automatic; minimizes datasource count while avoiding nonsense | fuzzy heuristics can misclassify; hard to predict; needs override | Higher | +| **4. User-specified grouping config** | CLI/JSON mapping tables → target datasources + per-group options | fully explicit, reproducible, arbitrary edge cases | user burden; verbose for batch; needs discovery tooling | Moderate | + +**Recommended shape:** default to **(2) merge-by-region** (or **(1) one-per-table** as the safest +default), with **(4) config override**, and **(3) as an opt-in `--auto-group`**. All four converge +on the same refactor. + +### Concrete refactor touch-points + +- Merge block → per-group concat + per-group `convert_scanpy_to_mdv`: + [conversion.py:1099–1131](../../../python/mdvtools/spatial/conversion.py). +- Table-collection loop (attach grouping key while iterating): + [conversion.py:1057–1077](../../../python/mdvtools/spatial/conversion.py). +- Regions-metadata write (currently single-obs-ds hardcoded — must target the right obs ds per + group): [conversion.py:1191–1203](../../../python/mdvtools/spatial/conversion.py) + + `project_helpers.build_spatial_regions_metadata` + ([project_helpers.py:11](../../../python/mdvtools/spatial/project_helpers.py)). +- New arg on `SpatialDataConversionArgs` + expose on click CLI + argparse. +- Default-view template assumes one obs/var pair: `spatial_view_template.json`, + `set_default_spatial_image_view` ([project_helpers.py:29](../../../python/mdvtools/spatial/project_helpers.py)) + — needs per-group views or a chosen "primary" datasource. +- Reusable helpers already present: `_allocate_table_prefix` (conversion.py:911), + `_sanitize_table_prefix` (conversion.py:906), `get_table_keys` (region/instance_key). + +### Prior art to mine + +- **`annotations.py`** (`patch-spatial-annotations`) already uses `instance_key` as a join key + ([annotations.py:96](../../../python/mdvtools/spatial/annotations.py)) — the pattern a + merge-by-region mode would reuse for structural row→element association. (It's "V1": requires + exactly one table and row-order match.) +- **`xenium.py`** (`convert_xenium_to_mdv`) handles a single Xenium store's elements directly — + self-described as needing review, but mineable for per-element handling ideas. +- **The user's other conversion scripts** (mentioned as living in other repos — e.g. under + `~/code/spatialdata`, `~/code/py/sd_sketch`) should be pulled in here for comparison before + fixing the grouping policy; they likely already encode grouping decisions worth adopting. +- `conversion_report.py` is a batch harness (`python -m mdvtools.spatial.conversion` over a + directory) — use it to regression-test any new merge modes against the existing corpus under + `data/spatialdata_conversion_report_*`. + +## Relationship to the JS DataLoader (theme 1) + +If theme 1 lands, a chunk of this converter's job — flattening tables into MDV columns ahead of +time — could instead happen **at read time in the browser**. The grouping *policy* (which tables +are which entity type, how they link to elements) is still needed either way; it just moves from +"bake into h5" to "describe in datasource metadata the JS loader consumes." Designing the grouping +model cleanly now pays off in both places. See [README.md](README.md) → "the radical vision." diff --git a/docs/design/spatial-tables/03-shared-spatial-contexts.md b/docs/design/spatial-tables/03-shared-spatial-contexts.md new file mode 100644 index 000000000..d26b50d00 --- /dev/null +++ b/docs/design/spatial-tables/03-shared-spatial-contexts.md @@ -0,0 +1,122 @@ +# 03 — Project-scoped shared contexts (image / store cache) + +> Why every spatial chart currently re-opens the zarr store and re-fetches images, and the safe +> path to a project-scoped cache. This is the enabling substrate for theme 1's DataLoader, and +> (per your framing) "one of the safer things to address." + +## The goal + +Two spatial charts (or a chart and its layer dialog) pointing at the same SpatialData path should +share **one opened store** and **one image cache**, instead of each independently calling +`readZarr` and re-loading pixels. + +## Why it's duplicated today + +**Every MDV chart and dialog is an independent React island.** `BaseReactChart.mountReact()` → +`createMdvPortal()` does a fresh `createRoot` per chart/dialog +([src/react/components/BaseReactChart.tsx:76](../../../src/react/components/BaseReactChart.tsx), +[src/react/react_utils.tsx:98](../../../src/react/react_utils.tsx)). There is no shared root; the +class docstring flags "a single root with portals" as unfinished future work +([BaseReactChart.tsx:34](../../../src/react/components/BaseReactChart.tsx)). + +**But a project-scoped provider pattern already works.** `createMdvPortal` wraps every island in: + +```tsx + // module singleton, react_utils.tsx:78 + + + {component} +``` + +The Provider *elements* are duplicated per island, but their *values* are **singletons**, so the +same context value crosses island boundaries. This is the template. + +### Three cache layers, all per-island today + +| Layer | What | Where | Shared? | +|---|---|---|---| +| (a) `SpatialData` store (zarr) | `useMemo(() => readZarr(source), [source])` | upstream `SpatialDataProvider`, mounted at [SpatialDataMDVReactComponent.tsx:297](../../../src/react/components/SpatialDataMDVReactComponent.tsx) **and** [SpatialLayerDialogReactWrapper.tsx:30](../../../src/react/components/SpatialLayerDialogReactWrapper.tsx) | **No** — memo is per provider instance; chart tree + dialog tree each open their own | +| (b) store-instance caches | `parquetTableCache`, `obsIndices`, `varIndices`, … | live *on* the `SpatialData` object (`@spatialdata/core`) | No — each `readZarr` yields a distinct instance | +| (c) Viv image loader / loaded pixels | OME-Zarr multiscale data | owned by `useSpatialCanvasRendererFromLayerInputs` ([…Component.tsx:124](../../../src/react/components/SpatialDataMDVReactComponent.tsx)) | No — one hook instance per chart viewer | + +The **Image Layer Registry** +([src/react/spatialdata/image_layer_registry.ts](../../../src/react/spatialdata/image_layer_registry.ts)) +bridges (c) from a chart's viewer tree to *its own* layer dialog — it is a **within-one-chart** +bridge, not cross-chart. Chart B's dialog cannot see chart A's registry. + +Confirmed by grep: there is **no** `spatialStore`, `SpatialStoreProvider`, `spatialDataCache`, or +shared `readZarr` wrapper in `src/`. Two charts on the same `region.spatial.file` re-open and +re-fetch. + +## The provider/context inventory (what NOT to touch) + +| Context | Holds | Scope | Verdict | +|---|---|---|---| +| `QueryClientProvider` / `ChartManagerProvider` / `ProjectProvider` | singletons | app/project | **precedent to follow** | +| `VivProvider` / `vivStores` (zustand) | runtime channel/UI state (histograms, brush) | per-chart (and a fresh copy per dialog panel) | **do not share** — intentionally per-chart; for the SpatialData chart it's not even the source of truth (canonical state is MobX `renderStack`) | +| `SpatialDataProvider` (`spatialDataPromise`) | `readZarr(source)` | per provider instance | **the one to dedupe** | +| `SpatialAnnotationProvider` | MDV scatter/gate/selection deck layers | per-chart | keep per-chart (chart-specific overlays) | +| `ImageLayerContextProvider` / `SpatialImagePanelContext` | registry callbacks / per-panel write API | per image panel | keep panel-local | + +## Recommended path + +### Step 1 — project-scoped SpatialData store cache (safe, high payoff) + +- New module `src/react/spatialdata/spatial_store_cache.ts`: a `Map>` + keyed by resolved store URL (+ `selection` if ever used). One instance per project — a module + singleton mirroring `queryClient`, or hung off `chartManager` / `ProjectContext`. + - **Don't cache a rejected `readZarr` promise indefinitely.** If a load fails, a raw + `Map` would poison that URL — every later read returns the same failed promise + until eviction/reload. Either evict the entry on rejection (`p.catch(() => cache.delete(url))`) + or keep the *inflight* promise separate from the *fulfilled* `SpatialData` entry, so transient + store/network errors stay recoverable and a retry re-fetches. +- Add a thin `SpatialStoreProvider` inside `createMdvPortal` + ([react_utils.tsx:108](../../../src/react/react_utils.tsx)), alongside `ProjectProvider`, + exposing the cache. +- Replace the two direct `` usages with a wrapper that resolves the + store from the shared cache (or wrap upstream `SpatialDataProvider` so its `readZarr` is deduped + by URL). Result: N charts + dialogs on one path → **1 `readZarr`, 1 `SpatialData` instance, + shared `parquetTableCache` etc.** (layers (a) and (b) shared) with minimal surface area. + +This alone requires **no single-root refactor** — the shared value crosses islands via the +singleton, exactly like `queryClient`. + +### Step 2 — share the Viv image loader / loaded pixels (larger) + +Today loaded-image data is owned by one renderer hook and exposed via the per-chart registry. To +reuse pixel/multiscale loads across charts you need a project-scoped image-loader cache keyed by +`(store, elementKey, selection)`, then have each chart's renderer consult it. This likely needs an +upstream `@spatialdata/vis` affordance (the integration doc already flags a "registry accessor on +composed renderer" gap, [docs/spatialdata-vis-integration.md:159](../../spatialdata-vis-integration.md)). +Higher risk — do after step 1. + +### Step 3 — connect to the DataLoader (theme 1) + +The project-scoped `SpatialData` store from step 1 is the **same object** a JS DataLoader reads +table columns from. Placing the cache on ChartManager/Project scope makes it reachable from both +React (via context) and non-React DataLoader code (via `window.mdv.chartManager`), the way +`ProjectContext` is already consumed both ways. + +## Risks + +- **React island boundaries** — share via a **singleton value**, never via React tree ancestry. +- **MobX vs zustand** — canonical layer state is MobX (`config.renderStack`, + `renderStackGeneration`), runtime channel UI is zustand. Keep the store cache a **plain + singleton/Map**, *not* MobX-observable, or you risk re-render churn in the identity-sensitive + render-stack adapter ([render_stack_adapter.ts](../../../src/react/spatialdata/render_stack_adapter.ts)). +- **Lifetime / eviction** — a project-scoped cache outlives individual charts; define eviction on + project unload / ChartManager teardown, or accept session lifetime like `queryClient`. Watch for + leaks of large zarr/image data. +- **StrictMode double-invoke** — `createMdvPortal` renders under ``; populate the cache + in the memo/getter (idempotent by URL key), not in an effect. +- **Registry teardown timing** — the Image Layer Registry is set/cleared in an effect tied to + `renderStackGeneration`. If image loading moves to a shared owner (step 2), revisit who + publishes/tears down the registry so the dialog panel still resolves. + +## Why this is the safe first move + +The provider stack already demonstrates working project-scoped singletons crossing island +boundaries. A store cache is **additive**, follows that exact precedent, is orthogonal to the +MobX/zustand machinery, and (step 1) touches only two provider mount sites plus one new module — +no single-React-root refactor, no change to the perf-critical render-stack adapter. And it unblocks +theme 1. diff --git a/docs/design/spatial-tables/04-views-datasources-chartmanager.md b/docs/design/spatial-tables/04-views-datasources-chartmanager.md new file mode 100644 index 000000000..690eab2b1 --- /dev/null +++ b/docs/design/spatial-tables/04-views-datasources-chartmanager.md @@ -0,0 +1,184 @@ +# 04 — Views, DataSources, and ChartManager + +> Four related ideas, from cheapest to most radical: (B1) a datasource picker in "Add Chart", +> (B2) cross-datasource shared layout, (A) a new view kind that bypasses much of ChartManager, +> and (C) swapping the ChartManager at runtime onto new data. Plus the "radical vision" that ties +> them together. + +## Current orchestration (grounded) + +One `ChartManager` ([src/charts/ChartManager.js](../../../src/charts/ChartManager.js)) is a +de-facto singleton (`window.mdv.chartManager`, set at ChartManager.js:155; warns on a second +construction). It is built from **five inputs**: + +```js +new ChartManager(holderDiv, datasources[], dataLoader, config, listener) // project_bootstrap.tsx:232 +``` + +- Constructor creates **one `DataStore` per datasource** (`new DataStore(d.size, d, dataLoader)`, + ChartManager.js:183) and `this.dsIndex` (name→record). +- `_init(view)` (ChartManager.js:732) splits the content area into **one pane + one toolbar per + datasource**: `splitPane(contentDiv, {number: dsToView.length, …})` (ChartManager.js:771), where + `dsToView = Object.keys(viewData.dataSources)` (ChartManager.js:757). Each datasource toolbar's + "Add Chart" opens `new AddChartDialogReact(dataStore)` with the target **fixed** by which toolbar + was clicked (ChartManager.js:1626). +- A chart is bound to exactly one `DataStore` at construction: + `new chartType.class(ds.dataStore, div, config)` (ChartManager.js:2268); registered as + `charts[id] = {chart, dataSource: ds}` (ChartManager.js:2275); DOM parent is `ds.contentDiv`; + gridstack is keyed **per DataSource** (`Map`, + [GridstackManager.ts:39](../../../src/charts/GridstackManager.ts)). + +### The serialized view shape + +`views.json` is `viewName → View` ([ViewManager.ts:9](../../../src/charts/ViewManager.ts)): + +```ts +type View = { + dataSources: Record; // keyed by ds NAME + initialCharts: Record; // ds NAME -> configs + links?; viewImage?; +}; +``` + +**The critical fact: a chart config never names its datasource.** Its binding is *only* which +`initialCharts[dsName]` array it sits in. `getState()` re-materializes that binding from +`this.charts[id].dataSource` on save (ChartManager.js:1252). `addChart(dsName, config)` already +keys on a name. + +### The chart ↔ host coupling surface + +A chart needs very little from its host +([BaseChart.ts:118](../../../src/charts/BaseChart.ts)): +- Constructor `(dataStore, div, config)`. It holds `dataStore` + `config` + `div`; it does **not** + hold a ChartManager reference. +- Real coupling is to the **`DataStore`** (columns, `Dimension`s for filter/highlight, color/legend + helpers) — not to panes or toolbars. +- ChartManager injects menu icons and drag/resize *after* construction. +- Cross-datasource access is opt-in, pushed in via `setupLinks` / `_giveChartAccess` + (ChartManager.js:2362) — the chart's *primary* dataStore stays singular. + +The `ChartType` descriptor ([ChartTypes.ts:27](../../../src/charts/ChartTypes.ts)): +`{class, name, required?, params?, allow_user_add?}` drives the Add Chart dialog entirely. + +## (B1) Datasource picker in Add Chart — low risk + +`AddChartDialogReact` ([dialogs/AddChartDialogReact.tsx](../../../src/charts/dialogs/AddChartDialogReact.tsx)) +currently takes a fixed `dataStore`; the type list is built from `Object.values(BaseChart.types)` +filtered by `allow_user_add` and `required` columns (AddChartDialogReact.tsx:262). On submit it +calls `window.mdv.chartManager.addChart(dataStore.name, config, true)`. + +**Change:** add a datasource `