Add zarr codec support for JP2K/HTJ2K with Python writer implementation - #48
Merged
Conversation
… and name accordingly use basic fractal test-fixture
…re clearly separate fixture-related code
Contributor
|
Warning Review limit reached
More reviews will be available in 50 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds codec support and store-backed loading across the viewer stack, introduces a ChangesCodec runtime and fixture pipeline
Sequence Diagram(s)sequenceDiagram
participant App
participant CodecFixtureDemo
participant SpatialCanvasViewer
participant Worker
participant FixtureStore
App->>CodecFixtureDemo: open /codec
CodecFixtureDemo->>FixtureStore: load fixture or manifest URL
CodecFixtureDemo->>SpatialCanvasViewer: provide selected layers
SpatialCanvasViewer->>Worker: request chunk decode
Worker->>FixtureStore: read codec-backed chunk
Worker-->>SpatialCanvasViewer: decoded tile data
sequenceDiagram
participant CLI
participant recompress_spatialdata
participant EncoderPool
participant encode-plane.mjs
participant OpenJPH
CLI->>recompress_spatialdata: recompress source store
recompress_spatialdata->>EncoderPool: encode image chunk planes
EncoderPool->>encode-plane.mjs: send worker request
encode-plane.mjs->>OpenJPH: encode HTJ2K plane
OpenJPH-->>encode-plane.mjs: encoded bytes
encode-plane.mjs-->>EncoderPool: worker response
EncoderPool-->>recompress_spatialdata: encoded chunk
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (12)
python/spatialdata-codec-writer/tests/test_htj2k_encode.py (1)
44-49: ⚡ Quick winEnsure global pool cleanup runs even on test failure.
Wrap the body in
try/finallysoshutdown_encoder_pool()always executes and doesn’t leak process state across tests.Suggested patch
def test_encode_htj2k_plane_uses_global_pool() -> None: shutdown_encoder_pool() - plane = np.zeros((8, 8), dtype=np.uint16) - encoded = encode_htj2k_plane(plane, reversible=True, quality=0.0) - assert len(encoded) > 0 - shutdown_encoder_pool() + try: + plane = np.zeros((8, 8), dtype=np.uint16) + encoded = encode_htj2k_plane(plane, reversible=True, quality=0.0) + assert len(encoded) > 0 + finally: + shutdown_encoder_pool()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/spatialdata-codec-writer/tests/test_htj2k_encode.py` around lines 44 - 49, The test function test_encode_htj2k_plane_uses_global_pool() does not guarantee cleanup of the encoder pool when the test fails. Wrap the test body (from the plane initialization through the assertion) in a try/finally block, with the shutdown_encoder_pool() call placed in the finally block to ensure it executes regardless of whether the test passes or fails, preventing process state leakage across tests.scripts/encode-htj2k-plane.mjs (1)
74-74: ⚡ Quick winAlign default HTJ2K
qualitywith the vendored encoder script.This script defaults
qualityto100, whilepython/spatialdata-codec-writer/src/spatialdata_codec_writer/vendor/encode-plane.mjsdefaults to0. Keeping defaults consistent avoids drift between fixture-generation paths.Suggested fix
- const quality = request.quality ?? 100; + const quality = request.quality ?? 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/encode-htj2k-plane.mjs` at line 74, The quality parameter in the encode-htj2k-plane.mjs script is defaulting to 100, but the vendored encoder script at python/spatialdata-codec-writer/src/spatialdata_codec_writer/vendor/encode-plane.mjs defaults to 0. Change the default value in the quality constant assignment from 100 to 0 to maintain consistency across both scripts and prevent configuration drift between the fixture-generation paths.tests/integration/codecFixtures.test.ts (1)
93-93: Replaceas Uint16Arrayassertions with a runtime narrowing helper.The two type assertions at lines 93 and 124 bypass TypeScript's type safety and can hide mismatched tile buffer types. Create a helper function to validate the buffer type at runtime instead:
Refactor
+function firstUint16Sample(tileData: unknown): number { + if (!(tileData instanceof Uint16Array)) { + throw new Error(`Expected Uint16Array tile data, got ${Object.prototype.toString.call(tileData)}`); + } + return Number(tileData[0]); +} + ... - expect(Number((tile.data as Uint16Array)[0])).toBe(manifest.chunks_checked[0].samples[0]); + expect(firstUint16Sample(tile.data)).toBe(manifest.chunks_checked[0].samples[0]); ... - expect(Number((tile.data as Uint16Array)[0])).toBe(manifest.chunks_checked[0].samples[0]); + expect(firstUint16Sample(tile.data)).toBe(manifest.chunks_checked[0].samples[0]);This strengthens test reliability by catching type mismatches and adheres to the TypeScript guideline: avoid assertions in favor of narrowing helpers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/codecFixtures.test.ts` at line 93, Create a runtime type-narrowing helper function that validates whether a buffer is an instance of Uint16Array (or the appropriate type) and either returns the properly typed buffer or throws an error. Replace the `as Uint16Array` type assertions at lines 93 and 124 with calls to this helper function. This ensures type safety is enforced at runtime rather than bypassed through assertions, making the test more reliable by catching actual type mismatches.Source: Coding guidelines
packages/vis/demo/src/CodecFixtureDemo.tsx (1)
309-311: ⚡ Quick winAvoid casting select values to
CodecFixtureKind.Use a small type guard so the state update is narrowed from runtime values without
as.Proposed fix
+function isCodecFixtureKind(value: string): value is CodecFixtureKind { + return value === 'jpeg2k' || value === 'htj2k'; +} ... <select value={fixtureKind} - onChange={(event) => setFixtureKind(event.target.value as CodecFixtureKind)} + onChange={(event) => { + const value = event.target.value; + if (isCodecFixtureKind(value)) setFixtureKind(value); + }} style={selectStyle} >As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and use narrowers/type guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vis/demo/src/CodecFixtureDemo.tsx` around lines 309 - 311, The onChange handler in the select element is using a type assertion (as CodecFixtureKind) to cast event.target.value, which violates coding guidelines. Create a type guard function that validates whether a value is a valid CodecFixtureKind, then use that guard in the onChange handler for setFixtureKind instead of the type assertion. This ensures runtime validation of the select value without relying on unsafe casts.Source: Coding guidelines
packages/zarrextra/tests/codecs.spec.ts (1)
46-47: ⚡ Quick winReplace repeated
as ...assertions with local type guards/helpers.The current assertions are avoidable in this file and make type safety dependent on casts instead of runtime narrowing.
Example pattern to remove assertions
+function isUint8Chunk(value: unknown): value is zarr.Chunk<'uint8'> { + return ( + !!value && + typeof value === 'object' && + 'shape' in value && + 'data' in value + ); +} ... -const arrAfter = await zarr.open(createCodecArrayStore(codecName) as zarr.Readable, { +const arrAfter = await zarr.open(createCodecArrayStore(codecName), { kind: 'array', }); const chunk = await zarr.get(arrAfter, [null, null]); -expect((chunk as zarr.Chunk<'uint8'>).shape).toEqual([2, 2]); +if (!isUint8Chunk(chunk)) throw new Error('Expected uint8 chunk'); +expect(chunk.shape).toEqual([2, 2]);As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and prefer narrowers/local guards.Also applies to: 62-64, 96-97, 113-114, 129-130, 163-164, 191-192, 208-210, 246-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/tests/codecs.spec.ts` around lines 46 - 47, Remove all `as ...` type assertions throughout the file (appearing at lines around 46-47, 62-64, 96-97, 113-114, 129-130, 163-164, 191-192, 208-210, and 246-248) and replace them with proper runtime type guards or local helper functions. Create type guard functions that validate the type at runtime before using the values, instead of relying on casts with `as`. For example, where `createCodecArrayStore` is cast with `as zarr.Readable`, implement a type guard function that checks if the store satisfies the Readable interface requirements, then use that guard before passing the value to zarr.open, ensuring type safety is achieved through narrowing rather than assertions.Source: Coding guidelines
packages/zarrextra/src/htj2k-encode.ts (1)
36-38: ⚡ Quick winApply guarded narrowing instead of
ascasts in loader/runtime extraction paths.The current assertions in the import/factory path hide shape mismatches until runtime. Prefer local type guards and typed helper extractors for these boundaries.
As per coding guidelines,
**/*.{ts,tsx}: “Avoid type assertions (as ...)… use local type guards or narrower API contracts.”Also applies to: 89-89, 118-118, 123-123, 126-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/src/htj2k-encode.ts` around lines 36 - 38, Remove the type assertion `as (specifier: string) => Promise<Record<string, unknown>>` from the dynamicImport function declaration. Instead, create a type guard or validation helper function that checks the shape of dynamically imported modules at their usage points (the loader/factory extraction paths around lines 89, 118, 123, 126). Replace the unsafe `as` casts at those locations with calls to your type guard to safely narrow and validate the imported module structure before accessing its properties.Source: Coding guidelines
packages/zarrextra/src/codecs.ts (1)
89-91: 🏗️ Heavy liftReplace broad
asassertions with explicit runtime narrowers at codec boundaries.This module currently relies on repeated
ascasts for dynamic imports/registry writes; that weakens type safety in exactly the external-boundary paths this file is handling. Please switch these to small type guards (or keep a single boundary assertion with an explanatory comment when narrowing is truly impossible).As per coding guidelines,
**/*.{ts,tsx}: “Avoid type assertions (as ...)… Keep assertions local to external boundaries with explanatory comments.”Also applies to: 241-247, 262-267, 290-293, 309-317, 334-340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/src/codecs.ts` around lines 89 - 91, The dynamicImport function definition uses a broad `as` type assertion that bypasses type safety at an external boundary without validation or explanation. Replace this assertion by either creating a runtime type guard function that validates the dynamicImport result matches the expected function signature before casting, or if a narrowing guard is not feasible, keep the assertion but add an explanatory comment documenting why the assertion is necessary at this codec boundary. Apply this same fix pattern to the other `as` assertions in the file at the specified line ranges (241-247, 262-267, 290-293, 309-317, 334-340) where codec registry writes and dynamic imports occur, ensuring type safety is strengthened throughout the external-boundary paths in the module.Source: Coding guidelines
packages/zarrextra/src/chunkDecode.ts (1)
68-68: ⚡ Quick winReplace
as zarr.Chunk<D>casts with a type guard + stronger generic contract.These assertions weaken type guarantees at the boundary. A local predicate plus a generic
GetWorkerFncan keep the same runtime checks withoutascasts.As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and prefer narrowers or tighter contracts.Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/src/chunkDecode.ts` at line 68, The return statements in chunkDecode.ts contain unsafe type assertions using `as zarr.Chunk<D>` at two locations. Replace these assertions by creating a type guard function that validates the result at runtime, and strengthen the generic contract using GetWorkerFn to ensure proper type constraints. This will eliminate the reliance on type casts while maintaining runtime safety through explicit type narrowing instead of assertions.Source: Coding guidelines
packages/zarrextra/tests/chunkDecode.spec.ts (1)
45-45: ⚡ Quick winDrop the chunk assertion casts in expectations.
At Line 45 and Line 67, use
chunk.datadirectly instead of(chunk as zarr.Chunk<'uint8'>).datato keep tests aligned with strict typing rules.As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and prefer precise typing/narrowing.Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/tests/chunkDecode.spec.ts` at line 45, Remove the type assertion casts from the expectation statements in the test file. At both locations (line 45 and line 67) where the code currently uses (chunk as zarr.Chunk<'uint8'>).data, replace it with just chunk.data to access the data property directly without explicit type casting. This aligns the test code with strict typing guidelines that discourage the use of type assertions.Source: Coding guidelines
packages/zarrextra/tests/omeZarr.spec.ts (1)
72-72: ⚡ Quick winRemove
asassertions in this test path.Line 72 and Line 80 use assertions that can mask contract drift. Prefer typing the store at creation time and narrowing
tile.datawith a runtime guard (instanceof Uint8Array) before use.As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and use narrower contracts/type guards instead.Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/tests/omeZarr.spec.ts` at line 72, Remove the `as zarr.Readable` type assertion from the call to `createOmeZarrStore()` on line 72 and the related assertion on line 80. Instead, ensure that `createOmeZarrStore()` is properly typed to return a value that implements the `zarr.Readable` contract at creation time. For narrowing `tile.data` before use, replace any `as` assertions with runtime type guards using `instanceof Uint8Array` to verify the type at runtime rather than asserting it.Source: Coding guidelines
packages/avivatorish/src/omeZarrMultiscales.ts (1)
4-9: ⚡ Quick winUse a typed
storecontract instead ofunknown+ascast.Line 17 currently relies on assertion to satisfy
loadOmeZarrMultiscalesFromStore. This weakens the boundary and defers bad input failures deeper into runtime.Suggested refactor
export type OmeZarrMultiscalesSource = | string | { url?: string; - store?: unknown; + store?: Parameters<typeof loadOmeZarrMultiscalesFromStore>[0]; }; @@ if (typeof source !== 'string' && source.store) { - return await loadOmeZarrMultiscalesFromStore( - source.store as Parameters<typeof loadOmeZarrMultiscalesFromStore>[0] - ); + return await loadOmeZarrMultiscalesFromStore(source.store); }As per coding guidelines,
**/*.{ts,tsx}should avoid type assertions (as ...) and use narrower API contracts or local guards.Also applies to: 15-18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/avivatorish/src/omeZarrMultiscales.ts` around lines 4 - 9, The `store` property in the `OmeZarrMultiscalesSource` type definition is currently typed as `unknown`, which requires a type assertion when passed to `loadOmeZarrMultiscalesFromStore`. Replace the `unknown` type with the actual typed contract expected by `loadOmeZarrMultiscalesFromStore` function. Identify the correct store type that the function accepts, update the type definition in `OmeZarrMultiscalesSource`, and remove the `as` assertion from the code that calls `loadOmeZarrMultiscalesFromStore` (lines 15-18). This will enforce proper type checking at the boundary and prevent runtime failures from invalid input.Source: Coding guidelines
packages/vis/demo/src/fixtureUrls.ts (1)
11-55: ⚡ Quick winDeduplicate base-origin fallback logic across fixture URL helpers.
All helpers repeat the same base computation. A tiny shared helper reduces drift and makes future host/port changes safer.
♻️ Proposed refactor
+function getDemoBaseOrigin(origin?: string): string { + return ( + origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173') + ); +} + export function getLocalBlobsFixtureUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/v${LOCAL_BLOBS_FIXTURE_VERSION}/blobs.zarr`; } @@ export function getLocalJpeg2kCodecFixtureUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/jpeg2k.zarr`; } @@ export function getLocalJpeg2kCodecManifestUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/jpeg2k.manifest.json`; } @@ export function getLocalHtj2kEncodeDemoManifestUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/htj2k-encode-demo.manifest.json`; } @@ export function getLocalHtj2kEncodeDemoFixtureUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/htj2k-demo.zarr`; } @@ export function getLocalHtj2kCodecFixtureUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/htj2k.zarr`; } @@ export function getLocalHtj2kCodecManifestUrl(origin?: string): string { - const base = - origin ?? (typeof window !== 'undefined' ? window.location.origin : 'http://127.0.0.1:5173'); + const base = getDemoBaseOrigin(origin); return `${base}/test-fixtures/codecs/htj2k.manifest.json`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vis/demo/src/fixtureUrls.ts` around lines 11 - 55, Multiple fixture URL helper functions (getLocalJpeg2kCodecFixtureUrl, getLocalJpeg2kCodecManifestUrl, getLocalHtj2kEncodeDemoManifestUrl, getLocalHtj2kEncodeDemoFixtureUrl, getLocalHtj2kCodecFixtureUrl, getLocalHtj2kCodecManifestUrl) all duplicate the same base origin fallback logic. Create a single shared helper function that computes the base URL from an optional origin parameter and the window location origin fallback, then replace the duplicated base computation in each fixture URL helper with a call to this shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/vis/codec-fixtures.mdx`:
- Line 153: The command on line 153 contains a machine-specific absolute path
`/Users/ptodd/data/spatialdata/sdata_inputs/xenium_rep1_io_spatialdata_0.7.1.zarr`
that is not portable and exposes personal environment details. Replace this
personal absolute path with a neutral, generic placeholder such as
`/path/to/input.zarr` to make the example reusable for all readers regardless of
their system configuration.
In `@packages/vis/demo/src/CodecFixtureDemo.tsx`:
- Around line 273-275: When the fixtureKind check (if fixtureKind !== 'htj2k')
triggers an early return, the HTJ2K-specific state variables htj2kDemoError and
htj2kDemo are not being cleared, leaving stale data that can persist when
switching to JP2K mode. Before the early return statement in the condition where
fixtureKind is not 'htj2k', add code to clear both htj2kDemoError and htj2kDemo
state variables (set them to null or their initial empty state). Apply the same
fix to the similar code block mentioned at lines 317-322 to ensure consistent
state cleanup when switching away from HTJ2K mode.
In `@packages/zarrextra/README.md`:
- Around line 100-106: Remove the `await` keyword from the
`enableWorkerChunkDecode()` function call in the README example code. Since
`enableWorkerChunkDecode()` is a synchronous function that returns a
`WorkerPool` directly rather than a Promise, the `await` operator should not be
used. Simply change `await enableWorkerChunkDecode();` to
`enableWorkerChunkDecode();` in the code snippet to accurately reflect the
synchronous nature of the API.
In `@packages/zarrextra/src/chunkDecode.ts`:
- Around line 59-64: The getWorkerImpl call in the worker backend path (lines
59-64) is not passing the abort signal from opts.signal, while the main-thread
path at line 71 correctly forwards it. To fix this, add the signal property from
opts.signal to the options object passed to getWorkerImpl so that cancellation
requests are properly propagated to the worker, ensuring stale tile decoding
stops when the request is aborted.
In `@packages/zarrextra/src/omeZarr.ts`:
- Around line 211-212: The current code at line 211 and line 229 assumes the
last 2 dimensions of result.shape are always height and width by using
slice(-2), but for interleaved arrays where the shape is [..., y, x, c], the
last 2 dimensions are actually width and channel. Determine if the result array
is interleaved (check if it has a channel dimension) and adjust the shape
slicing accordingly: for non-interleaved arrays use slice(-2) to get the last 2
dimensions [height, width], but for interleaved arrays use slice(-3, -1) to skip
the channel dimension at the end and get [height, width] instead. Apply this
logic consistently at both locations where this dimension extraction occurs.
- Around line 191-195: The bounds check logic around xStart, yStart, xStop, and
yStop only validates zero-sized slices (where start equals stop) but misses
inverted ranges where xStart is greater than xStop or yStart is greater than
yStop. These invalid cases can escape the BoundsCheckError handling and fail
later. Add additional conditions to the bounds checking to catch xStart > xStop
and yStart > yStop, throwing BoundsCheckError for these inverted range cases
alongside the existing zero-sized and out-of-bounds checks.
- Around line 258-260: Before accessing data[0] to call guessTileSize, add a
guard clause to check if the data array returned from loadMultiscales is not
empty. If data is empty, throw an error with a descriptive message indicating
that the multiscale dataset is empty or invalid. This prevents an unhelpful
"Cannot read property '0' of undefined" crash and provides clear feedback when
the metadata contains no valid datasets.
In `@packages/zarrextra/src/workers/codec-worker.ts`:
- Around line 14-26: The static import statement for
'`@fideus-labs/fizarrita/codec-worker`' is being hoisted and evaluated before the
codec registration calls in the module body. To ensure the codec registrations
(registerJpeg2kCodec and registerExperimentalHtj2kCodec) execute before the
worker loads, convert the static import to a dynamic import using await import()
and place it as the last statement after all registration calls are complete,
rather than using a static import statement which gets hoisted automatically.
In `@packages/zarrextra/tests/codecs.spec.ts`:
- Around line 174-193: The test `wrapZarrRegistryForFizarritaWorker adapts
built-in codecs to fizarrita metadata` calls
wrapZarrRegistryForFizarritaWorker() which mutates the global zarr.registry, but
unlike other tests in this file, it does not restore the registry state after
execution. Add a finally block to the test that saves the original registry
state before calling wrapZarrRegistryForFizarritaWorker() and restores it in the
finally block to prevent the mutation from affecting subsequent tests and avoid
test order dependency issues.
In `@python/spatialdata-codec-writer/docs/htj2k-wasm-encode-design.md`:
- Around line 26-32: The fenced code block containing the flow diagram lacks a
language identifier, which violates markdownlint rule MD040. Add a language tag
`text` immediately after the opening triple backticks (``` text) to properly
label the code fence. This applies to the diagram block showing the Python
spatialdata-codec-writer flow with EncoderPool, encode-plane.mjs, and
HTJ2KEncoder components.
In `@python/spatialdata-codec-writer/scripts/generate_codec_fixtures.py`:
- Around line 11-15: The guard condition at line 12 checks only if the scripts
directory `_SCRIPTS_DIR` is in `sys.path`, but it does not verify whether the
src directory path is present. This means if `_SCRIPTS_DIR` is already in the
path for any reason, the src path may not be added, causing import failures for
spatialdata_codec_writer. Modify the guard condition to check if the src
directory path (computed as `_SCRIPTS_DIR.parent / "src"`) is missing from
`sys.path`, and ensure the src path is always inserted when it's not already
present, independent of whether the scripts directory is already in the path.
In `@python/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.py`:
- Around line 23-30: The validation check in the condition starting with `if
args.quality is not None and args.codec == "imagecodecs_jpeg2k"` only rejects
the explicit JPEG2000 codec, but does not handle the case where --codec is
omitted and defaults to a non-HTJ2K codec. Instead of checking if codec equals
the JPEG2000 variant, change the condition to enforce that when args.quality is
provided, the codec must explicitly be set to "experimental.openjph_htj2k".
Replace the equality check with a negation check to ensure the codec is not the
HTJ2K codec whenever --quality is present.
- Around line 107-111: The --workers argument in the recompress.add_argument()
call currently accepts zero and negative values, which should be rejected at
argument parsing time. Add a custom type validator function to the type
parameter that ensures the value is a positive integer greater than zero, and
raise a TypeError or ValueError with a descriptive message if a non-positive
value is provided, so invalid inputs are caught immediately during argument
parsing.
In `@python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py`:
- Around line 88-103: The functions chunk_grid() and chunk_slices() use zip()
which silently truncates when input tuple lengths differ, potentially producing
incorrect chunk mappings. Add validation at the beginning of chunk_grid() to
ensure shape and chunks parameters have equal length, and add validation at the
beginning of chunk_slices() to ensure shape, chunks, and coords parameters all
have equal length. These validations should raise an appropriate exception (such
as ValueError) with a descriptive message if the lengths do not match, allowing
the code to fail fast rather than producing silently incorrect results.
In
`@python/spatialdata-codec-writer/src/spatialdata_codec_writer/htj2k_encode.py`:
- Around line 71-88: The HTJ2K encoder worker subprocess has two lifecycle
issues that can cause hangs and leaked processes. First, the subprocess.Popen
call creates a stderr pipe that is never consumed, which can block the Node
worker due to pipe buffer limits - either redirect stderr to subprocess.DEVNULL
or combine it with stdout using subprocess.STDOUT. Second, in the close method,
the self._proc.wait(timeout=5) call will not forcefully terminate the process if
it doesn't exit within the timeout, potentially leaving zombie processes - add a
check after wait() to call self._proc.terminate() or self._proc.kill() if the
process is still running (check with self._proc.poll()).
- Around line 154-172: The issue is that `get_encoder_pool()` does not respect
the worker configuration previously set by `configure_encoder_pool()`. When
`get_encoder_pool()` is called without arguments (workers=None), it should use
the stored `_pool_workers` value that was configured, but instead it always
passes `workers=None` to the EncoderPool constructor. Modify the instantiation
logic in `get_encoder_pool()` so that when creating a new EncoderPool, if the
workers parameter is None, it uses the stored `_pool_workers` value instead,
ensuring that the configuration set by `configure_encoder_pool()` is properly
applied to default encode calls.
In `@python/spatialdata-codec-writer/src/spatialdata_codec_writer/recompress.py`:
- Around line 319-346: The code materializes all chunks in memory by storing
every padded plane in chunk_jobs and every encoded payload+plane in
encoded_by_coords before any are persisted, causing O(total raster size) peak
memory usage. Additionally, the parallel path has an O(n²) lookup when searching
chunk_jobs with the next() call for each completed future. To fix this, refactor
to avoid pre-computing and storing all chunks: for the single-threaded path in
the first if block, process and yield chunks directly without storing them in
chunk_jobs; for the parallel path in the else block, map futures directly to
their corresponding planes in a dictionary when submitting to the executor
(instead of mapping coords), eliminating the expensive next() lookup, and
process each future's result immediately without storing all encoded results in
encoded_by_coords before the loop completes.
In `@scripts/vendor-openjph-for-python.mjs`:
- Around line 39-43: Before the for loop that copies the files, add preflight
validation to check that all required artifacts exist at their source paths.
Verify that both 'openjphjs.js' and 'openjphjs.wasm' exist in the
openjphRoot/dist/ directory using existsSync or similar check before proceeding
with the copyFileSync operations. If any required artifact is missing, throw an
error and exit early to prevent partial vendor state. Only after all source
files are confirmed to exist should the for loop proceed with copying them to
the vendor destination.
In `@tests/integration/codecFixtures.test.ts`:
- Around line 24-33: The ensureCodecFixture function gates the early return on
both hasJpeg2k and hasHtj2k existing, which causes repeated execSync calls when
HTJ2K fixtures cannot be generated. Change the condition at line 27 from
requiring both fixtures to requiring only hasJpeg2k, so the function returns
early if the essential JPEG2K fixture already exists, regardless of HTJ2K
generation status. This prevents the slow and unstable repeated execSync
invocations when HTJ2K generation fails or is unavailable.
---
Nitpick comments:
In `@packages/avivatorish/src/omeZarrMultiscales.ts`:
- Around line 4-9: The `store` property in the `OmeZarrMultiscalesSource` type
definition is currently typed as `unknown`, which requires a type assertion when
passed to `loadOmeZarrMultiscalesFromStore`. Replace the `unknown` type with the
actual typed contract expected by `loadOmeZarrMultiscalesFromStore` function.
Identify the correct store type that the function accepts, update the type
definition in `OmeZarrMultiscalesSource`, and remove the `as` assertion from the
code that calls `loadOmeZarrMultiscalesFromStore` (lines 15-18). This will
enforce proper type checking at the boundary and prevent runtime failures from
invalid input.
In `@packages/vis/demo/src/CodecFixtureDemo.tsx`:
- Around line 309-311: The onChange handler in the select element is using a
type assertion (as CodecFixtureKind) to cast event.target.value, which violates
coding guidelines. Create a type guard function that validates whether a value
is a valid CodecFixtureKind, then use that guard in the onChange handler for
setFixtureKind instead of the type assertion. This ensures runtime validation of
the select value without relying on unsafe casts.
In `@packages/vis/demo/src/fixtureUrls.ts`:
- Around line 11-55: Multiple fixture URL helper functions
(getLocalJpeg2kCodecFixtureUrl, getLocalJpeg2kCodecManifestUrl,
getLocalHtj2kEncodeDemoManifestUrl, getLocalHtj2kEncodeDemoFixtureUrl,
getLocalHtj2kCodecFixtureUrl, getLocalHtj2kCodecManifestUrl) all duplicate the
same base origin fallback logic. Create a single shared helper function that
computes the base URL from an optional origin parameter and the window location
origin fallback, then replace the duplicated base computation in each fixture
URL helper with a call to this shared helper.
In `@packages/zarrextra/src/chunkDecode.ts`:
- Line 68: The return statements in chunkDecode.ts contain unsafe type
assertions using `as zarr.Chunk<D>` at two locations. Replace these assertions
by creating a type guard function that validates the result at runtime, and
strengthen the generic contract using GetWorkerFn to ensure proper type
constraints. This will eliminate the reliance on type casts while maintaining
runtime safety through explicit type narrowing instead of assertions.
In `@packages/zarrextra/src/codecs.ts`:
- Around line 89-91: The dynamicImport function definition uses a broad `as`
type assertion that bypasses type safety at an external boundary without
validation or explanation. Replace this assertion by either creating a runtime
type guard function that validates the dynamicImport result matches the expected
function signature before casting, or if a narrowing guard is not feasible, keep
the assertion but add an explanatory comment documenting why the assertion is
necessary at this codec boundary. Apply this same fix pattern to the other `as`
assertions in the file at the specified line ranges (241-247, 262-267, 290-293,
309-317, 334-340) where codec registry writes and dynamic imports occur,
ensuring type safety is strengthened throughout the external-boundary paths in
the module.
In `@packages/zarrextra/src/htj2k-encode.ts`:
- Around line 36-38: Remove the type assertion `as (specifier: string) =>
Promise<Record<string, unknown>>` from the dynamicImport function declaration.
Instead, create a type guard or validation helper function that checks the shape
of dynamically imported modules at their usage points (the loader/factory
extraction paths around lines 89, 118, 123, 126). Replace the unsafe `as` casts
at those locations with calls to your type guard to safely narrow and validate
the imported module structure before accessing its properties.
In `@packages/zarrextra/tests/chunkDecode.spec.ts`:
- Line 45: Remove the type assertion casts from the expectation statements in
the test file. At both locations (line 45 and line 67) where the code currently
uses (chunk as zarr.Chunk<'uint8'>).data, replace it with just chunk.data to
access the data property directly without explicit type casting. This aligns the
test code with strict typing guidelines that discourage the use of type
assertions.
In `@packages/zarrextra/tests/codecs.spec.ts`:
- Around line 46-47: Remove all `as ...` type assertions throughout the file
(appearing at lines around 46-47, 62-64, 96-97, 113-114, 129-130, 163-164,
191-192, 208-210, and 246-248) and replace them with proper runtime type guards
or local helper functions. Create type guard functions that validate the type at
runtime before using the values, instead of relying on casts with `as`. For
example, where `createCodecArrayStore` is cast with `as zarr.Readable`,
implement a type guard function that checks if the store satisfies the Readable
interface requirements, then use that guard before passing the value to
zarr.open, ensuring type safety is achieved through narrowing rather than
assertions.
In `@packages/zarrextra/tests/omeZarr.spec.ts`:
- Line 72: Remove the `as zarr.Readable` type assertion from the call to
`createOmeZarrStore()` on line 72 and the related assertion on line 80. Instead,
ensure that `createOmeZarrStore()` is properly typed to return a value that
implements the `zarr.Readable` contract at creation time. For narrowing
`tile.data` before use, replace any `as` assertions with runtime type guards
using `instanceof Uint8Array` to verify the type at runtime rather than
asserting it.
In `@python/spatialdata-codec-writer/tests/test_htj2k_encode.py`:
- Around line 44-49: The test function
test_encode_htj2k_plane_uses_global_pool() does not guarantee cleanup of the
encoder pool when the test fails. Wrap the test body (from the plane
initialization through the assertion) in a try/finally block, with the
shutdown_encoder_pool() call placed in the finally block to ensure it executes
regardless of whether the test passes or fails, preventing process state leakage
across tests.
In `@scripts/encode-htj2k-plane.mjs`:
- Line 74: The quality parameter in the encode-htj2k-plane.mjs script is
defaulting to 100, but the vendored encoder script at
python/spatialdata-codec-writer/src/spatialdata_codec_writer/vendor/encode-plane.mjs
defaults to 0. Change the default value in the quality constant assignment from
100 to 0 to maintain consistency across both scripts and prevent configuration
drift between the fixture-generation paths.
In `@tests/integration/codecFixtures.test.ts`:
- Line 93: Create a runtime type-narrowing helper function that validates
whether a buffer is an instance of Uint16Array (or the appropriate type) and
either returns the properly typed buffer or throws an error. Replace the `as
Uint16Array` type assertions at lines 93 and 124 with calls to this helper
function. This ensures type safety is enforced at runtime rather than bypassed
through assertions, making the test more reliable by catching actual type
mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca44d821-5a79-4891-b7f2-133d408695a3
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpython/spatialdata-codec-writer/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
.github/workflows/test.yml.gitignoreAGENTS.mddocs/docs/vis/codec-fixtures.mdxdocs/docs/vis/headless-viewer.mdxpackage.jsonpackages/avivatorish/package.jsonpackages/avivatorish/src/omeZarrMultiscales.tspackages/avivatorish/tsconfig.jsonpackages/avivatorish/vite.config.tspackages/core/src/models/index.tspackages/vis/README.mdpackages/vis/demo/src/App.tsxpackages/vis/demo/src/CodecFixtureDemo.tsxpackages/vis/demo/src/enableDemoWorkerChunkDecode.tspackages/vis/demo/src/fixtureUrls.tspackages/vis/demo/src/main.tsxpackages/vis/demo/tsconfig.jsonpackages/vis/package.jsonpackages/vis/src/SpatialCanvas/VivLoaderRegistry.tsxpackages/vis/src/SpatialCanvas/renderers/imageRenderer.tspackages/vis/vite.config.demo.tspackages/zarrextra/README.mdpackages/zarrextra/package.jsonpackages/zarrextra/src/chunkDecode.tspackages/zarrextra/src/codecs.tspackages/zarrextra/src/htj2k-encode.tspackages/zarrextra/src/index.tspackages/zarrextra/src/omeZarr.tspackages/zarrextra/src/prefixedStore.tspackages/zarrextra/src/workers/codec-worker.tspackages/zarrextra/src/workers/index.tspackages/zarrextra/tests/chunkDecode.spec.tspackages/zarrextra/tests/codecs.spec.tspackages/zarrextra/tests/omeZarr.spec.tspackages/zarrextra/tests/workers.spec.tspackages/zarrextra/vite.config.tspython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/docs/htj2k-wasm-encode-design.mdpython/spatialdata-codec-writer/pyproject.tomlpython/spatialdata-codec-writer/scripts/fixture_writer.pypython/spatialdata-codec-writer/scripts/generate_codec_fixtures.pypython/spatialdata-codec-writer/scripts/htj2k_fixtures.pypython/spatialdata-codec-writer/scripts/provenance.pypython/spatialdata-codec-writer/scripts/synthetic_images.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/__init__.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/htj2k_encode.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/recompress.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/vendor/__init__.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/vendor/encode-plane.mjspython/spatialdata-codec-writer/tests/conftest.pypython/spatialdata-codec-writer/tests/test_htj2k_encode.pypython/spatialdata-codec-writer/tests/test_htj2k_encode_demo.pypython/spatialdata-codec-writer/tests/test_htj2k_quality.pypython/spatialdata-codec-writer/tests/test_recompress.pypython/spatialdata-codec-writer/tests/test_synthetic_images.pypython/spatialdata-codec-writer/tests/test_writer.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.tsvite.config.base.ts
…rror handling in ZarrPixelSource. Update CodecFixtureDemo to reset demo states on fixture kind change. Add codec worker initialization for OpenJPH and OpenJPEG. Update README for worker chunk decode usage.
… writer. Refactor HTJ2K encoding quality parameter, enhance chunk processing, and ensure consistent error reporting. Adjust test cases for encoder pool and codec registration.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/zarrextra/vite.config.ts (1)
25-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
external()can still externalizezarritafrom the codec-worker dependency graph.Line 26 only matches importers containing
codec-worker, but transitive imports (for example throughsrc/codecs.ts) fall through to Line 32 and may emit a barezarritaimport incodec-worker.js. That can break default worker loading vianew URL('./codec-worker.js', import.meta.url)in browser module workers.Suggested fix
external(id, parentId) { - if (parentId?.includes('codec-worker')) { + const importer = parentId ?? ''; + const isCodecWorkerGraph = + /[/\\]src[/\\]workers[/\\]codec-worker(?:-init)?\.ts$/.test(importer) || + /[/\\]src[/\\]codecs\.ts$/.test(importer); + if (isCodecWorkerGraph) { return false; } - if (parentId?.includes('workers')) { + if (/[/\\]src[/\\]workers[/\\]/.test(importer)) { return pkgExternals.includes(id); } return id === 'zarrita'; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/vite.config.ts` around lines 25 - 33, The external() function currently only prevents externalizing zarrita for direct imports in codec-worker, but transitive imports (like imports through src/codecs.ts) fall through and get externalized. To fix this, add an additional check before the final return statement that prevents externalizing zarrita when parentId includes 'workers' (since codec-worker is in the workers directory). This ensures zarrita is never externalized from any part of the codec-worker dependency graph, whether imported directly or transitively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/zarrextra/vite.config.ts`:
- Around line 25-33: The external() function currently only prevents
externalizing zarrita for direct imports in codec-worker, but transitive imports
(like imports through src/codecs.ts) fall through and get externalized. To fix
this, add an additional check before the final return statement that prevents
externalizing zarrita when parentId includes 'workers' (since codec-worker is in
the workers directory). This ensures zarrita is never externalized from any part
of the codec-worker dependency graph, whether imported directly or transitively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79cf785f-c427-4259-8b4b-d3f78addc510
📒 Files selected for processing (22)
docs/docs/vis/codec-fixtures.mdxpackages/vis/demo/src/CodecFixtureDemo.tsxpackages/zarrextra/README.mdpackages/zarrextra/src/chunkDecode.tspackages/zarrextra/src/omeZarr.tspackages/zarrextra/src/workers/codec-worker-init.tspackages/zarrextra/src/workers/codec-worker.tspackages/zarrextra/tests/codecs.spec.tspackages/zarrextra/vite.config.tspython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/docs/htj2k-wasm-encode-design.mdpython/spatialdata-codec-writer/scripts/generate_codec_fixtures.pypython/spatialdata-codec-writer/scripts/provenance.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/htj2k_encode.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/recompress.pypython/spatialdata-codec-writer/tests/test_htj2k_encode.pypython/spatialdata-codec-writer/tests/test_recompress.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/docs/vis/codec-fixtures.mdx
- python/spatialdata-codec-writer/docs/htj2k-wasm-encode-design.md
- packages/zarrextra/README.md
- python/spatialdata-codec-writer/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- python/spatialdata-codec-writer/scripts/provenance.py
- scripts/vendor-openjph-for-python.mjs
- packages/zarrextra/tests/codecs.spec.ts
- python/spatialdata-codec-writer/tests/test_htj2k_encode.py
- packages/vis/demo/src/CodecFixtureDemo.tsx
- scripts/encode-htj2k-plane.mjs
- tests/integration/codecFixtures.test.ts
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py
- python/spatialdata-codec-writer/scripts/generate_codec_fixtures.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/htj2k_encode.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/recompress.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/zarrextra/tests/omeZarr.spec.ts (1)
179-205: Consolidate scatteredasassertions using type guards and boundary helpers.The repeated
as zarr.Readableandas Uint8Arrayassertions bypass type checks unnecessarily. Replace with a local type guard (isUint8Array) for tile data narrowing and a single boundary helper (toReadableStore) for store conversions. This aligns with the coding guideline to avoid assertions where narrowers can express the same fact.Type guards before indexing:
- Use
instanceof Uint8Arraycheck before accessingtile.data[index]- Throw a clear error if the assertion fails in tests
Boundary assertion:
- Localize
as zarr.Readablein atoReadableStore()helper with a comment explaining why the cast is unavoidableExample refactor
+function isUint8Array(value: unknown): value is Uint8Array { + return value instanceof Uint8Array; +} + +function toReadableStore(store: Map<string, Uint8Array>): zarr.Readable { + // Test boundary: in-memory Map fixture used as Zarrita-readable store. + return store as unknown as zarr.Readable; +} + it('returns distinct tiles for different z and t selections', async () => { const [source] = await loadOmeZarrMultiscalesFromStore( - createMultiTzOmeZarrStore() as zarr.Readable + toReadableStore(createMultiTzOmeZarrStore()) ); @@ - expect((tileTz0.data as Uint8Array)[0]).toBe(indexedVolumeValue(0, 0, 0, 0, 0)); - expect((tileTz1.data as Uint8Array)[0]).toBe(indexedVolumeValue(1, 0, 2, 0, 0)); - expect((tileTz0.data as Uint8Array)[0]).not.toBe((tileTz1.data as Uint8Array)[0]); + if (!isUint8Array(tileTz0.data) || !isUint8Array(tileTz1.data)) { + throw new Error('Expected Uint8Array tile data for uint8 fixture.'); + } + expect(tileTz0.data[0]).toBe(indexedVolumeValue(0, 0, 0, 0, 0)); + expect(tileTz1.data[0]).toBe(indexedVolumeValue(1, 0, 2, 0, 0)); + expect(tileTz0.data[0]).not.toBe(tileTz1.data[0]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/zarrextra/tests/omeZarr.spec.ts` around lines 179 - 205, The test file has multiple repeated type assertions using `as zarr.Readable` and `as Uint8Array` that bypass type safety unnecessarily. Create a type guard function `isUint8Array` that checks if tile.data is an instance of Uint8Array, and use it before accessing indices on tile.data throughout the test (in tileTz0, tileTz1, explicit, and defaulted tile references). Additionally, create a boundary helper function `toReadableStore` that wraps the store conversion from createMultiTzOmeZarrStore and createZcyxOmeZarrStore with a single `as zarr.Readable` cast, eliminating the repeated assertions in the loadOmeZarrMultiscalesFromStore calls. Ensure the type guard throws a clear error message if the assertion fails to maintain test integrity.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/spatialdata-codec-writer/scripts/synthetic_images.py`:
- Around line 114-117: The code currently uses an if-else pattern where the
condition checks if pattern equals "indexed", but the else clause silently falls
back to _mandelbulb_plane for any unrecognized pattern value. Replace this
implicit default behavior with explicit validation by adding a check after the
if-else block that raises a ValueError if the pattern variable is not one of the
supported options. This will prevent typos or invalid pattern values from
silently generating incorrect output and will make errors visible to callers
immediately.
---
Nitpick comments:
In `@packages/zarrextra/tests/omeZarr.spec.ts`:
- Around line 179-205: The test file has multiple repeated type assertions using
`as zarr.Readable` and `as Uint8Array` that bypass type safety unnecessarily.
Create a type guard function `isUint8Array` that checks if tile.data is an
instance of Uint8Array, and use it before accessing indices on tile.data
throughout the test (in tileTz0, tileTz1, explicit, and defaulted tile
references). Additionally, create a boundary helper function `toReadableStore`
that wraps the store conversion from createMultiTzOmeZarrStore and
createZcyxOmeZarrStore with a single `as zarr.Readable` cast, eliminating the
repeated assertions in the loadOmeZarrMultiscalesFromStore calls. Ensure the
type guard throws a clear error message if the assertion fails to maintain test
integrity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d44b1a2-ae30-4532-b90e-7244faef9df5
📒 Files selected for processing (9)
.cursor/settings.json.vscode/settings.jsonpackages/zarrextra/src/omeZarr.tspackages/zarrextra/tests/omeZarr.spec.tspython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/spatialdata-codec-writer/scripts/synthetic_images.pypython/spatialdata-codec-writer/tests/test_recompress.pypython/spatialdata-codec-writer/tests/test_synthetic_images.py
✅ Files skipped from review due to trivial changes (3)
- .vscode/settings.json
- python/spatialdata-codec-writer/pyproject.toml
- python/spatialdata-codec-writer/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/zarrextra/src/omeZarr.ts
- python/spatialdata-codec-writer/tests/test_recompress.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/docs/vis/codec-fixtures.mdx (1)
172-173: 💤 Low valueUse a portable placeholder path instead of a system-specific directory.
Line 172 uses
/private/tmp, which is UNIX-specific. Windows users reading this example would need to adapt it toC:\tempor similar. For broader cross-platform applicability, use a neutral placeholder like/path/to/tmpor/tmp.Suggested replacement
-bunx http-server --cors -c-1 /private/tmp +bunx http-server --cors -c-1 /path/to/tmp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/vis/codec-fixtures.mdx` around lines 172 - 173, The command example using `bunx http-server --cors -c-1 /private/tmp` contains a UNIX-specific path that Windows users cannot directly use. Replace the `/private/tmp` directory path with a cross-platform placeholder such as `/path/to/tmp` or `/tmp` to make the documentation example accessible to users on all operating systems, allowing them to easily substitute their own temporary directory path as needed.tests/integration/codecFixtures.test.ts (1)
133-136: Replaceas Uint16Arraywith a runtime type guard for safety.The
tile.dataproperty is typed asunknownin theVivCompatiblePixelSourceinterface, making type assertions unsafe. Extract a small helper that usesinstanceofto narrow the type at runtime instead of asserting.Proposed fix
+function firstUint16Sample(data: unknown): number { + if (!(data instanceof Uint16Array)) { + throw new Error(`Expected Uint16Array tile data, got ${Object.prototype.toString.call(data)}`); + } + return Number(data[0]); +} ... - expect(Number((tileTz0.data as Uint16Array)[0])).toBe(manifest.chunks_checked[0].samples[0]); - expect(Number((tileTz1.data as Uint16Array)[0])).not.toBe( - Number((tileTz0.data as Uint16Array)[0]) + expect(firstUint16Sample(tileTz0.data)).toBe(manifest.chunks_checked[0].samples[0]); + expect(firstUint16Sample(tileTz1.data)).not.toBe( + firstUint16Sample(tileTz0.data) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/codecFixtures.test.ts` around lines 133 - 136, The test file uses unsafe type assertions with `as Uint16Array` to cast the `data` property from `unknown` type. Instead of asserting the type, create a small helper function that uses the `instanceof` operator to perform a runtime type guard that safely narrows the `tile.data` property to `Uint16Array`. Then replace all instances of the `as Uint16Array` assertions (in the `tileTz0.data` and `tileTz1.data` expressions) with calls to this new helper function to ensure type safety at runtime.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/spatialdata-codec-writer/scripts/write_synthetic.py`:
- Around line 33-36: The _tczyx_chunks function validates that exactly five
integers are provided but does not validate that each integer is positive. Add
validation logic after the arity check to ensure all values in the input list
are greater than zero, and raise an argparse.ArgumentTypeError with a
descriptive message if any value is zero or negative. This will prevent invalid
chunk sizes from being accepted and causing failures downstream.
In
`@python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py`:
- Around line 9-17: The main function in synthetic_cli.py is attempting to
dynamically manipulate sys.path to import from scripts/write_synthetic.py, but
the scripts directory is not included in the package distribution, causing
import failures in installed packages. Remove the sys.path manipulation logic
(the lines that construct scripts_dir, convert to string, and call
sys.path.insert) and move the implementation from scripts/write_synthetic.py
into the spatialdata_codec_writer package itself (create it as a module within
src/spatialdata_codec_writer/), then update the import statement in the main
function to import directly from the new package location instead of from the
external scripts directory.
---
Nitpick comments:
In `@docs/docs/vis/codec-fixtures.mdx`:
- Around line 172-173: The command example using `bunx http-server --cors -c-1
/private/tmp` contains a UNIX-specific path that Windows users cannot directly
use. Replace the `/private/tmp` directory path with a cross-platform placeholder
such as `/path/to/tmp` or `/tmp` to make the documentation example accessible to
users on all operating systems, allowing them to easily substitute their own
temporary directory path as needed.
In `@tests/integration/codecFixtures.test.ts`:
- Around line 133-136: The test file uses unsafe type assertions with `as
Uint16Array` to cast the `data` property from `unknown` type. Instead of
asserting the type, create a small helper function that uses the `instanceof`
operator to perform a runtime type guard that safely narrows the `tile.data`
property to `Uint16Array`. Then replace all instances of the `as Uint16Array`
assertions (in the `tileTz0.data` and `tileTz1.data` expressions) with calls to
this new helper function to ensure type safety at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a4dacfb-9862-4b66-9a54-c0f32f8be8b7
📒 Files selected for processing (16)
docs/docs/vis/codec-fixtures.mdxpackage.jsonpackages/vis/README.mdpackages/vis/demo/src/CodecFixtureDemo.tsxpackages/vis/demo/src/fixtureUrls.tspython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/spatialdata-codec-writer/scripts/generate_codec_fixtures.pypython/spatialdata-codec-writer/scripts/mandelbulb_fixtures.pypython/spatialdata-codec-writer/scripts/synthetic_images.pypython/spatialdata-codec-writer/scripts/write_synthetic.pypython/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.pypython/spatialdata-codec-writer/tests/test_synthetic_images.pypython/spatialdata-codec-writer/tests/test_write_synthetic.pypython/spatialdata-codec-writer/tests/test_writer.pytests/integration/codecFixtures.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/vis/README.md
- python/spatialdata-codec-writer/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- python/spatialdata-codec-writer/pyproject.toml
- package.json
- python/spatialdata-codec-writer/scripts/generate_codec_fixtures.py
- python/spatialdata-codec-writer/scripts/synthetic_images.py
- python/spatialdata-codec-writer/tests/test_synthetic_images.py
- python/spatialdata-codec-writer/tests/test_writer.py
Comment on lines
+33
to
+36
| def _tczyx_chunks(values: list[int]) -> tuple[int, int, int, int, int]: | ||
| if len(values) != 5: | ||
| raise argparse.ArgumentTypeError("chunks must contain exactly five integers: t c z y x") | ||
| return (values[0], values[1], values[2], values[3], values[4]) |
Contributor
There was a problem hiding this comment.
Validate --chunks elements as positive integers.
--chunks currently accepts any integers (Line 155), and _tczyx_chunks only validates arity (Line 33). Zero/negative chunk sizes slip through and fail later. Validate positivity at parse time so the CLI fails with a clear argument error.
Proposed fix
def _tczyx_chunks(values: list[int]) -> tuple[int, int, int, int, int]:
if len(values) != 5:
raise argparse.ArgumentTypeError("chunks must contain exactly five integers: t c z y x")
+ if any(v <= 0 for v in values):
+ raise argparse.ArgumentTypeError("chunks must be positive integers: t c z y x")
return (values[0], values[1], values[2], values[3], values[4])Also applies to: 155-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/spatialdata-codec-writer/scripts/write_synthetic.py` around lines 33 -
36, The _tczyx_chunks function validates that exactly five integers are provided
but does not validate that each integer is positive. Add validation logic after
the arity check to ensure all values in the input list are greater than zero,
and raise an argparse.ArgumentTypeError with a descriptive message if any value
is zero or negative. This will prevent invalid chunk sizes from being accepted
and causing failures downstream.
Comment on lines
+9
to
+17
| def main(argv: list[str] | None = None) -> None: | ||
| scripts_dir = Path(__file__).resolve().parents[2] / "scripts" | ||
| scripts_path = str(scripts_dir) | ||
| if scripts_path not in sys.path: | ||
| sys.path.insert(0, scripts_path) | ||
|
|
||
| from write_synthetic import main as write_synthetic_main | ||
|
|
||
| write_synthetic_main(argv) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate packaging metadata
fd -i pyproject.toml
# Inspect entrypoints and package include/exclude rules
rg -n -C3 "project\.scripts|console_scripts|synthetic_cli|write_synthetic|tool\.setuptools|include|package-data" pyproject.toml
# Confirm whether repo-local script exists where synthetic_cli expects it in-source
fd -a -i "write_synthetic.py"Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 282
🏁 Script executed:
# Read the pyproject.toml for the spatialdata-codec-writer package
cat python/spatialdata-codec-writer/pyproject.tomlRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 1128
🏁 Script executed:
# Search for write_synthetic.py more broadly
find . -name "write_synthetic.py" -o -name "write_synthetic*" 2>/dev/nullRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 136
🏁 Script executed:
# Check the directory structure around synthetic_cli.py
ls -la python/spatialdata-codec-writer/src/spatialdata_codec_writer/Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 622
🏁 Script executed:
# Check if scripts directory exists at the expected location
ls -la python/spatialdata-codec-writer/scripts/ 2>/dev/null || echo "scripts directory not found"Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 678
Remove the repo-path import dependency from the packaged CLI entrypoint.
The synthetic_cli.py entrypoint attempts to dynamically import from scripts/write_synthetic.py by manipulating sys.path. However, the scripts/ directory is not included in the package distribution (the pyproject.toml [tool.setuptools.package-data] section only includes vendor/ items, and setuptools.packages.find only looks in src/). When the package is installed normally, this command fails with an import error.
Either move the implementation into the spatialdata_codec_writer package directly or explicitly include scripts/ in package-data and update the import logic accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py`
around lines 9 - 17, The main function in synthetic_cli.py is attempting to
dynamically manipulate sys.path to import from scripts/write_synthetic.py, but
the scripts directory is not included in the package distribution, causing
import failures in installed packages. Remove the sys.path manipulation logic
(the lines that construct scripts_dir, convert to string, and call
sys.path.insert) and move the implementation from scripts/write_synthetic.py
into the spatialdata_codec_writer package itself (create it as a module within
src/spatialdata_codec_writer/), then update the import statement in the main
function to import directly from the new package location instead of from the
external scripts directory.
Merged
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
added a commit
that referenced
this pull request
Jul 31, 2026
* Read AnnData's nullable-encoded columns in the JS reader A nullable column is stored as a *group* of `values` + `mask`, not an array, so opening it as an array fails outright. Because `obs/_index` and `var/_index` are themselves columns, the visible symptom was missing variable names rather than a missing value — the browser fell back to `varN`. This is not confined to stores our writer has touched: AnnData writes nullable encodings by default from 0.13, and `spatialdata` inherits that, so freshly written stores carry them too. Handles all three encodings sharing the layout — `nullable-string-array`, `nullable-integer`, `nullable-boolean` — since they differ only in the dtype of `values`. The mask is applied rather than discarded, so a missing entry reads as `null` and stays distinguishable from an empty string or a real zero. Both read paths are covered: `_loadColumn`, which dispatches on `encoding-type`, and `getFlatArrDecompressed`, which index reads reach directly and which now resolves the node before assuming it is an array. A group with any other encoding still fails, with a message naming what was found. Not routed through `anndata.js`: it dispatches none of the nullable encodings (in the published 0.0.2 and on main), and it pins zarrita 0.5.1, which is the subject of the still-open upstream #48. Our index and column reads already deliberately bypass it for exactly this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Decode zarr v3 categorical columns to labels A categorical column's values were only decoded when its categories array had dtype `v2:object`. A zarr v3 store writes them as `string`, so the check failed and the column resolved to its raw integer codes — plausible looking numbers rather than an error, and wrong wherever a label was expected. v2 fixed-width unicode (`v2:U*`) had the same problem. Test against the type rather than one spelling of it: zarrita's `is()` already knows that `string`, `v2:U*` and `v2:S*` are all text, so the check becomes `is('string') || is('object')`. Also map pandas' -1 "missing" code to null instead of indexing off the end of the categories array and yielding undefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Report the declared kind of nullable obs columns `classifyObsColumnNode` settles a column's kind from its `encoding-type` where one is decisive, and otherwise falls back to the dtype in the node's array metadata. A nullable column is a group of `values` + `mask`, so it has no array metadata of its own, and its encoding was not in the lookup — every one of them came back `undefined`, which sends `'auto'` mode back to sniffing decoded values. That is not a rare shape. AnnData 0.13 defaults to zarr v3 and writes string columns as `nullable-string-array` there, so on a freshly written store the columns arriving without a declared kind are most of the text ones — the case the lookup exists to avoid. The three encoding names now live in `nullableArrays` alongside the reader, mapped to the kind of their `values`, because reading them and classifying them have to stay in step and are in different modules. Also switches `getObsColumnKinds` to the `getObsGroup()` helper. It was still reaching for `this.parsed.obs` behind a `typeof === 'object'` test, which admits a `LazyZarrArray` — the narrowing a08fd37 introduced for the two accessors either side of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
HTJ2K is a "high-throughput" variant on Jpeg2000 that is cheaper to encode and decode.
This can allow for lossless encoding, but also quite high compression-ratios with good quality in lossy modes.
Zarr doesn't generally support HTJ2K. Here we inject decoder implementations for both that and JP2K into
zarritaregistry (which is now also using workers throughfizarritato decode). We patchvivso that it uses astorereference rather than just aurl.JP2K is more recognised as a standard part of the zarr ecosystem, although in practice any store using that would likely be similarly broken for use with either codec.
Some basic facilities for generating synthetic fractal image data for testing.
Summary by CodeRabbit
Summary by CodeRabbit — Release Notes
New Features
Documentation
Tests
Chores