Consolidate Python tooling into a publishable spatialdata-js-util - #100
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR consolidates Python codec, image, point, table, verification, CLI, and TUI functionality into ChangesSpatialData utility consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/vendor-openjph-for-python.mjs (1)
31-41: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInclude the vendored OpenJPH helper script in package data.
encode-plane.mjsloadsvendor/openjph/index.mjsand the Python backend checks for bothindex.mjsandwasm/lib_openjph.wasm.pyproject.tomlonly packagescodecs/vendor/*.mjsandcodecs/vendor/openjph/*; addcodecs/vendor/*.mjs,codecs/vendor/openjph/index.mjs, and thewasm/*.wasmfiles so workers can locate the helper.🤖 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/vendor-openjph-for-python.mjs` around lines 31 - 41, The packaging configuration in pyproject.toml must include the vendored OpenJPH assets required by encode-plane.mjs and the Python backend: package the top-level codecs/vendor/*.mjs files, codecs/vendor/openjph/index.mjs, and codecs/vendor/openjph/wasm/*.wasm files. Update the relevant package-data inclusion rules without changing the vendorWasmDir path construction.
🧹 Nitpick comments (8)
python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs (1)
113-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-concatenating the whole pending buffer per stdin chunk.
Each stdin chunk copies the entire pending buffer. Encode requests carry base64 plane data, so one request can span many 64 KB chunks. The copy cost then grows quadratically with request size.
Accumulate chunks in a list and concatenate only when the declared frame length is available.
♻️ Proposed refactor
async function* readLengthPrefixedJson(stream) { - let buffer = Buffer.alloc(0); + const pending = []; + let pendingLength = 0; + let buffer = Buffer.alloc(0); for await (const chunk of stream) { - buffer = Buffer.concat([buffer, chunk]); + pending.push(chunk); + pendingLength += chunk.length; + if (buffer.length + pendingLength < 4) { + continue; + } + buffer = Buffer.concat([buffer, ...pending]); + pending.length = 0; + pendingLength = 0; while (buffer.length >= 4) { const length = buffer.readUInt32BE(0); if (buffer.length < 4 + length) { break; } const body = buffer.subarray(4, 4 + length); buffer = buffer.subarray(4 + length); yield JSON.parse(body.toString('utf8')); } } }🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs` around lines 113 - 127, Update readLengthPrefixedJson to accumulate incoming chunks without repeatedly copying the entire pending buffer: store chunks and track available bytes, parse the 4-byte length prefix when present, and concatenate only the chunks needed once the complete declared frame is available. Preserve support for frames split across chunks and continue yielding parsed JSON while retaining any trailing bytes for subsequent frames.python/spatialdata-js-util/src/spatialdata_js_util/points.py (1)
219-239: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRecord the applied sort order in the multiscale metadata.
write_multiscale_points_parquetsorts bygenefirst when that column exists, so__spatial_index__is not the primary key. The metadata frombuild_spatialdata_multiscale_metadatarecords axes, bounds, and levels, but not the sort order. A reader cannot tell which key is primary, and ADR 0002 states that the row-group bisect depends on the primary sort key.Add the applied sort key list to the metadata written at line 237.
🤖 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-js-util/src/spatialdata_js_util/points.py` around lines 219 - 239, The write_multiscale_points_parquet function must record its applied sort order in the metadata passed to _write_arrow_table_in_row_groups. Build metadata reflecting the actual sort_keys list, including gene as the primary key when present, and ensure the resulting metadata preserves the existing axes, bounds, and levels while adding the sort-key information.python/spatialdata-js-util/src/spatialdata_js_util/store.py (2)
275-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate branch in
read_points_dataframe.The
if path.is_dir()andelsebranches call the same expression.ds.datasethandles both a file and a directory, so the condition adds no behavior.♻️ Proposed simplification
def read_points_dataframe(parquet_path: str | Path) -> pd.DataFrame: path = Path(parquet_path) if not path.exists(): raise FileNotFoundError(f"Points Parquet not found: {path}") - if path.is_dir(): - table = ds.dataset(path, format="parquet").to_table() - else: - table = ds.dataset(path, format="parquet").to_table() - return table.to_pandas() + return ds.dataset(path, format="parquet").to_table().to_pandas()🤖 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-js-util/src/spatialdata_js_util/store.py` around lines 275 - 283, Remove the redundant path.is_dir() conditional in read_points_dataframe and call ds.dataset(path, format="parquet").to_table() once for both files and directories, preserving the existing existence check and pandas conversion.
246-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite the root metadata through
write_json.Line 272 serialises the root
zarr.jsonwithjson.dumps(doc, indent=2)and nosort_keys, whilewrite_jsonusessort_keys=True. The same file therefore gets two different key orders depending on which function last wrote it, which produces noisy diffs and defeats byte-comparison of store metadata. Also, the deep copies at lines 222 and 271 are easier to read ascopy.deepcopy.♻️ Proposed change
for key in element_keys: - metadata[f"points/{key}"] = json.loads(json.dumps(template_entry)) - root_json.write_text(json.dumps(doc, indent=2) + "\n") + metadata[f"points/{key}"] = deepcopy(template_entry) + write_json(root_json, doc)Add the import:
from copy import deepcopy🤖 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-js-util/src/spatialdata_js_util/store.py` around lines 246 - 272, Update register_points_elements_in_consolidated_metadata to persist the modified root document via the existing write_json helper instead of manually calling json.dumps and write_text, preserving consistent sorted metadata output. Replace the json.loads/json.dumps deep-copy expression used for template entries with copy.deepcopy, adding the deepcopy import and applying it to the existing copy sites in this file.python/spatialdata-js-util/src/spatialdata_js_util/verify.py (2)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix two static-analysis findings.
Line 34 names a parameter
id, which shadows the Python builtin (Ruff A002). Line 138 contains an EN DASH in2–4(Ruff RUF001). Rename the parameter and use a hyphen.♻️ Proposed change
-def _check(id: str, passed: bool, detail: str) -> VerifyCheck: - return VerifyCheck(id=id, passed=passed, detail=detail) +def _check(check_id: str, passed: bool, detail: str) -> VerifyCheck: + return VerifyCheck(id=check_id, passed=passed, detail=detail)- f"sentinel prefix rows: {sentinel_count} (expected 2–4)", + f"sentinel prefix rows: {sentinel_count} (expected 2-4)",Also applies to: 138-138
🤖 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-js-util/src/spatialdata_js_util/verify.py` around lines 34 - 35, Update the _check function’s id parameter and corresponding VerifyCheck construction to use a non-shadowing name while preserving the same value. In the line 138 text containing “2–4”, replace the en dash with a standard hyphen.Source: Linters/SAST tools
291-313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRecord a check when a directory dataset is skipped.
Line 292 accepts a directory dataset, but line 305 requires
parquet_path.is_file()before running the Morton checks. A multipart directory therefore produces only thepath_check and no Morton verification, andall_passedstill returnsTrue. Add an explicit skipped or failed check so the report shows that the Morton validation did not run.🤖 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-js-util/src/spatialdata_js_util/verify.py` around lines 291 - 313, Update the verification flow around parquet_path and verify_morton_parquet so directory datasets accepted by the path check receive an explicit check when Morton validation is skipped. Preserve Morton verification for file datasets, and record the skipped or failed outcome with an id tied to condition_id so all_passed reflects that validation did not run.python/spatialdata-js-util/tests/test_verify.py (1)
153-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
DEFAULT_CONDITIONSstatically.Line 9 already imports from
spatialdata_js_util.index_permutations. The__import__call at Line 156 repeats the module path as a string, so a future rename can update Line 9 and leave this string stale. Add the symbol to the existing import.Proposed change
-from spatialdata_js_util.index_permutations import write_index_permutations +from spatialdata_js_util.index_permutations import DEFAULT_CONDITIONS, write_index_permutationsconditions=tuple( condition - for condition in __import__( - "spatialdata_js_util.index_permutations", - fromlist=["DEFAULT_CONDITIONS"], - ).DEFAULT_CONDITIONS + for condition in DEFAULT_CONDITIONS if condition.id in {"canonical", "morton"} ),🤖 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-js-util/tests/test_verify.py` around lines 153 - 160, Update the existing import in test_verify.py to include DEFAULT_CONDITIONS, then replace the dynamic __import__ expression in the conditions tuple with the directly imported symbol while preserving the canonical and morton filtering.python/spatialdata-js-util/tests/test_synthetic_images.py (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe exact-equality reference test is sensitive to float operation order.
Line 33 compares the production plane with a scalar Python reference. The escape test
zr*zr + zi*zi <= 4.0decides an integer iteration count. If the production implementation reorders the same float operations or uses a different dtype, points near the boundary can differ by one iteration and the test fails without a real defect. Consider asserting near-equality with a small tolerance on iteration counts, or keep the reference and add a comment that both implementations must usefloat64and the same operation order.🤖 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-js-util/tests/test_synthetic_images.py` around lines 31 - 33, Update test_mandelbrot_plane_matches_reference_implementation to avoid brittle exact equality caused by floating-point operation order or dtype differences: compare the production and reference arrays with a small, explicit tolerance appropriate for integer iteration counts. Preserve the reference implementation and the existing size-32 coverage.
🤖 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-js-util/docs/multi-component-codec-findings.md`:
- Around line 133-135: Update the HTJ2K backend description in
multi-component-codec-findings.md to reflect that the runtime may select
imagecodecs when its multi-component probe succeeds, with openjph-wasm used as
the fallback; retain imagecodecs’s separate JPEG2000 fixture-path role.
In `@python/spatialdata-js-util/README.md`:
- Line 58: Update the fenced code block in the README to specify the `text`
language identifier, without changing its contents.
In `@python/spatialdata-js-util/scripts/benchmark_points_index.py`:
- Around line 27-35: The _feature_codes function returns None when no
scenario_id is provided, but _load_bounds defaults to the first
benchmark_scenarios entry in this case, creating an inconsistency where scenario
bounds are applied without the corresponding feature-code filter. Update
_feature_codes to default to the first scenario in the scenarios list when
scenario_id is None, matching the defaulting logic of _load_bounds, so that both
bounds and feature codes are derived from the same selected scenario.
In `@python/spatialdata-js-util/src/spatialdata_js_util/cli.py`:
- Around line 61-69: Update _recompress_chunks to raise the CLI error type
handled by _images_recompress instead of argparse.ArgumentTypeError, so invalid
chunk values such as non-integer axis sizes produce the command’s usage error
without a traceback. Preserve the existing “auto” handling and integer tuple
conversion.
In `@python/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.py`:
- Around line 67-99: Extend the HTJ2K backend coverage around
ImagecodecsHtj2kBackend and the existing backend_passes_probe so it exercises
encode followed by decode, not only decode. Add lossless and one or more lossy
round-trip checks, and compare the requested quality/level behavior from
ImagecodecsHtj2kBackend.encode against the WASM backend at matching lossy
settings.
In `@python/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.py`:
- Around line 68-70: The HTJ2K encoding path rejects the registered legacy codec
even though LegacyHtj2kCodec inherits _encode_single(). Update both encoding
functions covering the shown branches to accept is_htj2k_codec(codec), including
CODEC_HTJ2K_LEGACY, so legacy HTJ2K encoding works for Zarr writes while
preserving the existing unsupported-codec error.
In
`@python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 156-164: Update the main().catch error path to avoid calling
process.exit(1) immediately after writeResponse or process.stderr.write. Set
process.exitCode to 1 instead, allowing stdout to flush fully so the parent
receives the complete error frame before the process terminates.
In `@python/spatialdata-js-util/src/spatialdata_js_util/index_permutations.py`:
- Around line 84-102: Update _write_points_collection_zarr_json to detect
whether the source points group uses Zarr v2 by checking for points/.zgroup when
points/zarr.json is absent, then recreate the matching v2 group document instead
of writing a v3 zarr.json. Preserve copying the source zarr.json for v3 stores
and the existing early return when destination metadata already exists.
In `@python/spatialdata-js-util/src/spatialdata_js_util/pyramids.py`:
- Around line 97-100: Update the explicit-level validation in the pyramids logic
to raise WriterCommandError when levels exceeds MAX_LEVELS, rather than silently
clamping it in the return expression. Preserve the existing validation for
values below 1 and keep automatic-level handling unchanged.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tables.py`:
- Around line 198-205: Update _table_dimensions and _convert_table_matrices to
handle table groups without an X child before accessing root["X"]. Skip such
tables or raise the established WriterCommandError so the CLI reports a clear,
supported error instead of propagating KeyError; preserve existing behavior for
tables that contain X.
- Around line 228-240: Before the overwrite removal in the destination-handling
branch, resolve and compare source_path and store_path, rejecting destinations
that are identical to or contain the source store as a parent. Raise
WriterCommandError before shutil.rmtree or shutil.copytree so the source remains
intact, while preserving normal overwrite behavior for unrelated destinations.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tui/screens.py`:
- Around line 732-784: Update the INPUT_ORDER tuple in RecompressImagesScreen to
include the pyramid-related input field identifiers. The current tuple ends with
"workers", but should also include "pyramid-levels", "pyramid-downscale", and
"pyramid-min-size" in that order to ensure form navigation flows through all
input fields before submitting. Add these three identifiers to the end of the
INPUT_ORDER tuple, matching the ids used in the Input widgets defined in the
compose method.
- Around line 907-985: Update TablesToCscScreen._submit to check whether an
explicit dest already exists before creating the task; reject it with an error
notification when it exists and overwrite is false, and require ConfirmScreen
whenever it exists, regardless of overwrite. Preserve the existing in-place
confirmation behavior for dest is None, while allowing RunScreen only for
non-existing explicit destinations.
In `@python/spatialdata-js-util/src/spatialdata_js_util/verify.py`:
- Around line 202-220: Update verify_multiscale_parquet to catch exceptions from
pq.ParquetFile when reading the candidate file, following the error-handling
pattern in verify_morton_parquet. Return checks containing a failed verification
result with the read error details instead of propagating the exception, while
preserving the existing successful metadata validation flow.
In `@python/spatialdata-js-util/tests/test_tables.py`:
- Around line 223-242: Rename test_refresh_refuses_to_write_orphaned_entries to
describe successful reconstruction of the missing points parent, preserving its
current assertions. Add a separate test that creates a genuinely orphaned
consolidated-metadata entry, calls refresh_consolidated_metadata, and asserts
that it raises ValueError, thereby covering the ValueError branch in store.py.
In `@python/spatialdata-js-util/tests/test_tui.py`:
- Around line 216-241: Add the existing HTJ2K availability skip guard to
TestRecompressImages.test_recompresses_losslessly_and_reads_back and both
pyramid tests that press Run, before their TUI execution begins. Reuse the same
guard and availability symbol used by tests/test_recompress.py and
tests/test_zarr_codec.py; leave the validation-only tests unchanged.
In `@python/spatialdata-js-util/tests/test_zarr_codec.py`:
- Around line 46-48: The module-level skipif decorator on
test_zarr_array_round_trips_through_the_codec at line 46-48 blocks both
parametrized codec values based on a single HTJ2K availability check, causing
CODEC_JPEG2K to skip incorrectly when JPEG2K is available without HTJ2K, and to
run incorrectly when only HTJ2K is available. Remove the `@pytest.mark.skipif`
decorator from line 46-48 and add a per-codec availability check inside the
test_zarr_array_round_trips_through_the_codec function that skips the test based
on the codec_name parameter (using a helper that maps CODEC_HTJ2K_OPENJPH to
htj2k_available() and CODEC_JPEG2K to an appropriate JPEG2K availability check).
Apply the same per-codec availability check inside
test_lossless_recompressed_store_reads_back_exactly at lines 147-148 by removing
its module-level skipif decorator and adding equivalent conditional skip logic
based on codec_name.
- Around line 108-114: Update test_selected_backend_passes_the_probe to assert
backend_passes_probe(selected) whenever a backend is selected, rather than
conditionally checking only the BACKEND_IMAGECODECS report entry. Keep the
existing skip behavior when selected is None.
---
Outside diff comments:
In `@scripts/vendor-openjph-for-python.mjs`:
- Around line 31-41: The packaging configuration in pyproject.toml must include
the vendored OpenJPH assets required by encode-plane.mjs and the Python backend:
package the top-level codecs/vendor/*.mjs files,
codecs/vendor/openjph/index.mjs, and codecs/vendor/openjph/wasm/*.wasm files.
Update the relevant package-data inclusion rules without changing the
vendorWasmDir path construction.
---
Nitpick comments:
In
`@python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 113-127: Update readLengthPrefixedJson to accumulate incoming
chunks without repeatedly copying the entire pending buffer: store chunks and
track available bytes, parse the 4-byte length prefix when present, and
concatenate only the chunks needed once the complete declared frame is
available. Preserve support for frames split across chunks and continue yielding
parsed JSON while retaining any trailing bytes for subsequent frames.
In `@python/spatialdata-js-util/src/spatialdata_js_util/points.py`:
- Around line 219-239: The write_multiscale_points_parquet function must record
its applied sort order in the metadata passed to
_write_arrow_table_in_row_groups. Build metadata reflecting the actual sort_keys
list, including gene as the primary key when present, and ensure the resulting
metadata preserves the existing axes, bounds, and levels while adding the
sort-key information.
In `@python/spatialdata-js-util/src/spatialdata_js_util/store.py`:
- Around line 275-283: Remove the redundant path.is_dir() conditional in
read_points_dataframe and call ds.dataset(path, format="parquet").to_table()
once for both files and directories, preserving the existing existence check and
pandas conversion.
- Around line 246-272: Update register_points_elements_in_consolidated_metadata
to persist the modified root document via the existing write_json helper instead
of manually calling json.dumps and write_text, preserving consistent sorted
metadata output. Replace the json.loads/json.dumps deep-copy expression used for
template entries with copy.deepcopy, adding the deepcopy import and applying it
to the existing copy sites in this file.
In `@python/spatialdata-js-util/src/spatialdata_js_util/verify.py`:
- Around line 34-35: Update the _check function’s id parameter and corresponding
VerifyCheck construction to use a non-shadowing name while preserving the same
value. In the line 138 text containing “2–4”, replace the en dash with a
standard hyphen.
- Around line 291-313: Update the verification flow around parquet_path and
verify_morton_parquet so directory datasets accepted by the path check receive
an explicit check when Morton validation is skipped. Preserve Morton
verification for file datasets, and record the skipped or failed outcome with an
id tied to condition_id so all_passed reflects that validation did not run.
In `@python/spatialdata-js-util/tests/test_synthetic_images.py`:
- Around line 31-33: Update
test_mandelbrot_plane_matches_reference_implementation to avoid brittle exact
equality caused by floating-point operation order or dtype differences: compare
the production and reference arrays with a small, explicit tolerance appropriate
for integer iteration counts. Preserve the reference implementation and the
existing size-32 coverage.
In `@python/spatialdata-js-util/tests/test_verify.py`:
- Around line 153-160: Update the existing import in test_verify.py to include
DEFAULT_CONDITIONS, then replace the dynamic __import__ expression in the
conditions tuple with the directly imported symbol while preserving the
canonical and morton filtering.
🪄 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 Plus
Run ID: 4557de99-5a41-4d85-9c03-4dad9c354729
⛔ Files ignored due to path filters (2)
python/spatialdata-experimental-writer/uv.lockis excluded by!**/*.lockpython/spatialdata-js-util/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.cursor/settings.json.github/workflows/test.yml.gitignore.vscode/settings.jsondocs/adr/0002-spatially-aware-vector-loading.mddocs/docs/vis/codec-fixtures.mdxdocs/plans/shapes-nonblocking-tiled-loading.mdpackage.jsonpackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/parquetFooterStats.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/vtableDirectoryResponse.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/zarrextra/README.mdpython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/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/synthetic_cli.pypython/spatialdata-experimental-writer/README.mdpython/spatialdata-experimental-writer/pyproject.tomlpython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.pypython/spatialdata-js-util/README.mdpython/spatialdata-js-util/docs/htj2k-wasm-encode-design.mdpython/spatialdata-js-util/docs/multi-component-codec-findings.mdpython/spatialdata-js-util/pyproject.tomlpython/spatialdata-js-util/scripts/benchmark_points_index.pypython/spatialdata-js-util/scripts/build_htj2k_probe.pypython/spatialdata-js-util/scripts/fixture_writer.pypython/spatialdata-js-util/scripts/generate_codec_fixtures.pypython/spatialdata-js-util/scripts/htj2k_fixtures.pypython/spatialdata-js-util/scripts/mandelbulb_fixtures.pypython/spatialdata-js-util/scripts/provenance.pypython/spatialdata-js-util/scripts/synthetic_images.pypython/spatialdata-js-util/scripts/write_synthetic.pypython/spatialdata-js-util/src/spatialdata_js_util/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/cli.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/chunks.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/htj2k_wasm.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/names.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2cpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjspython/spatialdata-js-util/src/spatialdata_js_util/codecs/zarr_codec.pypython/spatialdata-js-util/src/spatialdata_js_util/errors.pypython/spatialdata-js-util/src/spatialdata_js_util/images.pypython/spatialdata-js-util/src/spatialdata_js_util/index_permutations.pypython/spatialdata-js-util/src/spatialdata_js_util/points.pypython/spatialdata-js-util/src/spatialdata_js_util/provenance.pypython/spatialdata-js-util/src/spatialdata_js_util/pyramids.pypython/spatialdata-js-util/src/spatialdata_js_util/runners.pypython/spatialdata-js-util/src/spatialdata_js_util/store.pypython/spatialdata-js-util/src/spatialdata_js_util/tables.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/app.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/models.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/screens.pypython/spatialdata-js-util/src/spatialdata_js_util/verify.pypython/spatialdata-js-util/tests/conftest.pypython/spatialdata-js-util/tests/test_htj2k_encode.pypython/spatialdata-js-util/tests/test_htj2k_encode_demo.pypython/spatialdata-js-util/tests/test_htj2k_quality.pypython/spatialdata-js-util/tests/test_integration.pypython/spatialdata-js-util/tests/test_points.pypython/spatialdata-js-util/tests/test_pyramids.pypython/spatialdata-js-util/tests/test_recompress.pypython/spatialdata-js-util/tests/test_synthetic_images.pypython/spatialdata-js-util/tests/test_tables.pypython/spatialdata-js-util/tests/test_tui.pypython/spatialdata-js-util/tests/test_verify.pypython/spatialdata-js-util/tests/test_write_synthetic.pypython/spatialdata-js-util/tests/test_writer.pypython/spatialdata-js-util/tests/test_zarr.pypython/spatialdata-js-util/tests/test_zarr_codec.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.ts
💤 Files with no reviewable changes (11)
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py
- python/spatialdata-experimental-writer/README.md
- python/spatialdata-codec-writer/pyproject.toml
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/init.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py
- python/spatialdata-experimental-writer/pyproject.toml
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/init.py
- python/spatialdata-codec-writer/README.md
Comment on lines
+68
to
+70
| if codec == CODEC_HTJ2K_OPENJPH: | ||
| return _encode_htj2k(array, encode_options) | ||
| raise ValueError(f"Unsupported image codec: {codec}") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make legacy HTJ2K encoding behavior explicit.
LegacyHtj2kCodec is registered and inherits _encode_single(). It passes CODEC_HTJ2K_LEGACY to these functions. Both functions reject that codec because they test only CODEC_HTJ2K_OPENJPH.
Accept is_htj2k_codec(codec) in both functions, or make LegacyHtj2kCodec decode-only and prevent it from being used for Zarr writes.
Also applies to: 84-95
🤖 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-js-util/src/spatialdata_js_util/codecs/encoding.py` around
lines 68 - 70, The HTJ2K encoding path rejects the registered legacy codec even
though LegacyHtj2kCodec inherits _encode_single(). Update both encoding
functions covering the shown branches to accept is_htj2k_codec(codec), including
CODEC_HTJ2K_LEGACY, so legacy HTJ2K encoding works for Zarr writes while
preserving the existing unsupported-codec error.
Comment on lines
+84
to
+102
| def _write_points_collection_zarr_json(source_path: Path, dest_path: Path) -> None: | ||
| """Recreate the `points/` group metadata that `_copy_store_shell` skipped. | ||
|
|
||
| Without it the collection exists as a directory with no node metadata, so its | ||
| elements are listed in consolidated metadata under a parent that is not — an | ||
| orphan that stops the whole store from opening once anything rebuilds that | ||
| metadata from disk. | ||
| """ | ||
| dest_json = dest_path / "points" / "zarr.json" | ||
| if dest_json.is_file(): | ||
| return | ||
| dest_json.parent.mkdir(parents=True, exist_ok=True) | ||
| source_json = source_path / "points" / "zarr.json" | ||
| if source_json.is_file(): | ||
| shutil.copy2(source_json, dest_json) | ||
| return | ||
| dest_json.write_text( | ||
| json.dumps({"attributes": {}, "zarr_format": 3, "node_type": "group"}, indent=2) + "\n" | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle a Zarr v2 source store in the fallback.
The fallback always writes a Zarr v3 group document. A Zarr v2 source store has points/.zgroup and no points/zarr.json, so this writes a v3 node into a v2 store. list_element_keys in store.py (lines 123-132) accepts .zgroup, so v2 stores are in scope for this package.
Detect the source format and recreate the matching group document.
🛠️ Proposed fix
def _write_points_collection_zarr_json(source_path: Path, dest_path: Path) -> None:
dest_json = dest_path / "points" / "zarr.json"
if dest_json.is_file():
return
dest_json.parent.mkdir(parents=True, exist_ok=True)
source_json = source_path / "points" / "zarr.json"
if source_json.is_file():
shutil.copy2(source_json, dest_json)
return
+ source_zgroup = source_path / "points" / ".zgroup"
+ if source_zgroup.is_file():
+ shutil.copy2(source_zgroup, dest_json.parent / ".zgroup")
+ source_zattrs = source_path / "points" / ".zattrs"
+ if source_zattrs.is_file():
+ shutil.copy2(source_zattrs, dest_json.parent / ".zattrs")
+ return
dest_json.write_text(
json.dumps({"attributes": {}, "zarr_format": 3, "node_type": "group"}, indent=2) + "\n"
)Run the following script to check whether this package writes or reads Zarr v2 point stores:
#!/bin/bash
# Description: Look for Zarr v2 handling in the package to confirm v2 sources are supported.
rg -n --glob '*.py' -C 4 '\.zgroup|zarr_format\D*2|zarr_format_of' python/spatialdata-js-util🧰 Tools
🪛 ast-grep (0.45.0)
[info] 100-100: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"attributes": {}, "zarr_format": 3, "node_type": "group"}, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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-js-util/src/spatialdata_js_util/index_permutations.py`
around lines 84 - 102, Update _write_points_collection_zarr_json to detect
whether the source points group uses Zarr v2 by checking for points/.zgroup when
points/zarr.json is absent, then recreate the matching v2 group document instead
of writing a v3 zarr.json. Preserve copying the source zarr.json for v3 stores
and the existing early return when destination metadata already exists.
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/vendor-openjph-for-python.mjs (1)
31-41: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInclude the vendored OpenJPH helper script in package data.
encode-plane.mjsloadsvendor/openjph/index.mjsand the Python backend checks for bothindex.mjsandwasm/lib_openjph.wasm.pyproject.tomlonly packagescodecs/vendor/*.mjsandcodecs/vendor/openjph/*; addcodecs/vendor/*.mjs,codecs/vendor/openjph/index.mjs, and thewasm/*.wasmfiles so workers can locate the helper.🤖 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/vendor-openjph-for-python.mjs` around lines 31 - 41, The packaging configuration in pyproject.toml must include the vendored OpenJPH assets required by encode-plane.mjs and the Python backend: package the top-level codecs/vendor/*.mjs files, codecs/vendor/openjph/index.mjs, and codecs/vendor/openjph/wasm/*.wasm files. Update the relevant package-data inclusion rules without changing the vendorWasmDir path construction.
🧹 Nitpick comments (8)
python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs (1)
113-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-concatenating the whole pending buffer per stdin chunk.
Each stdin chunk copies the entire pending buffer. Encode requests carry base64 plane data, so one request can span many 64 KB chunks. The copy cost then grows quadratically with request size.
Accumulate chunks in a list and concatenate only when the declared frame length is available.
♻️ Proposed refactor
async function* readLengthPrefixedJson(stream) { - let buffer = Buffer.alloc(0); + const pending = []; + let pendingLength = 0; + let buffer = Buffer.alloc(0); for await (const chunk of stream) { - buffer = Buffer.concat([buffer, chunk]); + pending.push(chunk); + pendingLength += chunk.length; + if (buffer.length + pendingLength < 4) { + continue; + } + buffer = Buffer.concat([buffer, ...pending]); + pending.length = 0; + pendingLength = 0; while (buffer.length >= 4) { const length = buffer.readUInt32BE(0); if (buffer.length < 4 + length) { break; } const body = buffer.subarray(4, 4 + length); buffer = buffer.subarray(4 + length); yield JSON.parse(body.toString('utf8')); } } }🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs` around lines 113 - 127, Update readLengthPrefixedJson to accumulate incoming chunks without repeatedly copying the entire pending buffer: store chunks and track available bytes, parse the 4-byte length prefix when present, and concatenate only the chunks needed once the complete declared frame is available. Preserve support for frames split across chunks and continue yielding parsed JSON while retaining any trailing bytes for subsequent frames.python/spatialdata-js-util/src/spatialdata_js_util/points.py (1)
219-239: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRecord the applied sort order in the multiscale metadata.
write_multiscale_points_parquetsorts bygenefirst when that column exists, so__spatial_index__is not the primary key. The metadata frombuild_spatialdata_multiscale_metadatarecords axes, bounds, and levels, but not the sort order. A reader cannot tell which key is primary, and ADR 0002 states that the row-group bisect depends on the primary sort key.Add the applied sort key list to the metadata written at line 237.
🤖 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-js-util/src/spatialdata_js_util/points.py` around lines 219 - 239, The write_multiscale_points_parquet function must record its applied sort order in the metadata passed to _write_arrow_table_in_row_groups. Build metadata reflecting the actual sort_keys list, including gene as the primary key when present, and ensure the resulting metadata preserves the existing axes, bounds, and levels while adding the sort-key information.python/spatialdata-js-util/src/spatialdata_js_util/store.py (2)
275-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate branch in
read_points_dataframe.The
if path.is_dir()andelsebranches call the same expression.ds.datasethandles both a file and a directory, so the condition adds no behavior.♻️ Proposed simplification
def read_points_dataframe(parquet_path: str | Path) -> pd.DataFrame: path = Path(parquet_path) if not path.exists(): raise FileNotFoundError(f"Points Parquet not found: {path}") - if path.is_dir(): - table = ds.dataset(path, format="parquet").to_table() - else: - table = ds.dataset(path, format="parquet").to_table() - return table.to_pandas() + return ds.dataset(path, format="parquet").to_table().to_pandas()🤖 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-js-util/src/spatialdata_js_util/store.py` around lines 275 - 283, Remove the redundant path.is_dir() conditional in read_points_dataframe and call ds.dataset(path, format="parquet").to_table() once for both files and directories, preserving the existing existence check and pandas conversion.
246-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite the root metadata through
write_json.Line 272 serialises the root
zarr.jsonwithjson.dumps(doc, indent=2)and nosort_keys, whilewrite_jsonusessort_keys=True. The same file therefore gets two different key orders depending on which function last wrote it, which produces noisy diffs and defeats byte-comparison of store metadata. Also, the deep copies at lines 222 and 271 are easier to read ascopy.deepcopy.♻️ Proposed change
for key in element_keys: - metadata[f"points/{key}"] = json.loads(json.dumps(template_entry)) - root_json.write_text(json.dumps(doc, indent=2) + "\n") + metadata[f"points/{key}"] = deepcopy(template_entry) + write_json(root_json, doc)Add the import:
from copy import deepcopy🤖 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-js-util/src/spatialdata_js_util/store.py` around lines 246 - 272, Update register_points_elements_in_consolidated_metadata to persist the modified root document via the existing write_json helper instead of manually calling json.dumps and write_text, preserving consistent sorted metadata output. Replace the json.loads/json.dumps deep-copy expression used for template entries with copy.deepcopy, adding the deepcopy import and applying it to the existing copy sites in this file.python/spatialdata-js-util/src/spatialdata_js_util/verify.py (2)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix two static-analysis findings.
Line 34 names a parameter
id, which shadows the Python builtin (Ruff A002). Line 138 contains an EN DASH in2–4(Ruff RUF001). Rename the parameter and use a hyphen.♻️ Proposed change
-def _check(id: str, passed: bool, detail: str) -> VerifyCheck: - return VerifyCheck(id=id, passed=passed, detail=detail) +def _check(check_id: str, passed: bool, detail: str) -> VerifyCheck: + return VerifyCheck(id=check_id, passed=passed, detail=detail)- f"sentinel prefix rows: {sentinel_count} (expected 2–4)", + f"sentinel prefix rows: {sentinel_count} (expected 2-4)",Also applies to: 138-138
🤖 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-js-util/src/spatialdata_js_util/verify.py` around lines 34 - 35, Update the _check function’s id parameter and corresponding VerifyCheck construction to use a non-shadowing name while preserving the same value. In the line 138 text containing “2–4”, replace the en dash with a standard hyphen.Source: Linters/SAST tools
291-313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRecord a check when a directory dataset is skipped.
Line 292 accepts a directory dataset, but line 305 requires
parquet_path.is_file()before running the Morton checks. A multipart directory therefore produces only thepath_check and no Morton verification, andall_passedstill returnsTrue. Add an explicit skipped or failed check so the report shows that the Morton validation did not run.🤖 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-js-util/src/spatialdata_js_util/verify.py` around lines 291 - 313, Update the verification flow around parquet_path and verify_morton_parquet so directory datasets accepted by the path check receive an explicit check when Morton validation is skipped. Preserve Morton verification for file datasets, and record the skipped or failed outcome with an id tied to condition_id so all_passed reflects that validation did not run.python/spatialdata-js-util/tests/test_verify.py (1)
153-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
DEFAULT_CONDITIONSstatically.Line 9 already imports from
spatialdata_js_util.index_permutations. The__import__call at Line 156 repeats the module path as a string, so a future rename can update Line 9 and leave this string stale. Add the symbol to the existing import.Proposed change
-from spatialdata_js_util.index_permutations import write_index_permutations +from spatialdata_js_util.index_permutations import DEFAULT_CONDITIONS, write_index_permutationsconditions=tuple( condition - for condition in __import__( - "spatialdata_js_util.index_permutations", - fromlist=["DEFAULT_CONDITIONS"], - ).DEFAULT_CONDITIONS + for condition in DEFAULT_CONDITIONS if condition.id in {"canonical", "morton"} ),🤖 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-js-util/tests/test_verify.py` around lines 153 - 160, Update the existing import in test_verify.py to include DEFAULT_CONDITIONS, then replace the dynamic __import__ expression in the conditions tuple with the directly imported symbol while preserving the canonical and morton filtering.python/spatialdata-js-util/tests/test_synthetic_images.py (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe exact-equality reference test is sensitive to float operation order.
Line 33 compares the production plane with a scalar Python reference. The escape test
zr*zr + zi*zi <= 4.0decides an integer iteration count. If the production implementation reorders the same float operations or uses a different dtype, points near the boundary can differ by one iteration and the test fails without a real defect. Consider asserting near-equality with a small tolerance on iteration counts, or keep the reference and add a comment that both implementations must usefloat64and the same operation order.🤖 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-js-util/tests/test_synthetic_images.py` around lines 31 - 33, Update test_mandelbrot_plane_matches_reference_implementation to avoid brittle exact equality caused by floating-point operation order or dtype differences: compare the production and reference arrays with a small, explicit tolerance appropriate for integer iteration counts. Preserve the reference implementation and the existing size-32 coverage.
🤖 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-js-util/docs/multi-component-codec-findings.md`:
- Around line 133-135: Update the HTJ2K backend description in
multi-component-codec-findings.md to reflect that the runtime may select
imagecodecs when its multi-component probe succeeds, with openjph-wasm used as
the fallback; retain imagecodecs’s separate JPEG2000 fixture-path role.
In `@python/spatialdata-js-util/README.md`:
- Line 58: Update the fenced code block in the README to specify the `text`
language identifier, without changing its contents.
In `@python/spatialdata-js-util/scripts/benchmark_points_index.py`:
- Around line 27-35: The _feature_codes function returns None when no
scenario_id is provided, but _load_bounds defaults to the first
benchmark_scenarios entry in this case, creating an inconsistency where scenario
bounds are applied without the corresponding feature-code filter. Update
_feature_codes to default to the first scenario in the scenarios list when
scenario_id is None, matching the defaulting logic of _load_bounds, so that both
bounds and feature codes are derived from the same selected scenario.
In `@python/spatialdata-js-util/src/spatialdata_js_util/cli.py`:
- Around line 61-69: Update _recompress_chunks to raise the CLI error type
handled by _images_recompress instead of argparse.ArgumentTypeError, so invalid
chunk values such as non-integer axis sizes produce the command’s usage error
without a traceback. Preserve the existing “auto” handling and integer tuple
conversion.
In `@python/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.py`:
- Around line 67-99: Extend the HTJ2K backend coverage around
ImagecodecsHtj2kBackend and the existing backend_passes_probe so it exercises
encode followed by decode, not only decode. Add lossless and one or more lossy
round-trip checks, and compare the requested quality/level behavior from
ImagecodecsHtj2kBackend.encode against the WASM backend at matching lossy
settings.
In `@python/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.py`:
- Around line 68-70: The HTJ2K encoding path rejects the registered legacy codec
even though LegacyHtj2kCodec inherits _encode_single(). Update both encoding
functions covering the shown branches to accept is_htj2k_codec(codec), including
CODEC_HTJ2K_LEGACY, so legacy HTJ2K encoding works for Zarr writes while
preserving the existing unsupported-codec error.
In
`@python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 156-164: Update the main().catch error path to avoid calling
process.exit(1) immediately after writeResponse or process.stderr.write. Set
process.exitCode to 1 instead, allowing stdout to flush fully so the parent
receives the complete error frame before the process terminates.
In `@python/spatialdata-js-util/src/spatialdata_js_util/index_permutations.py`:
- Around line 84-102: Update _write_points_collection_zarr_json to detect
whether the source points group uses Zarr v2 by checking for points/.zgroup when
points/zarr.json is absent, then recreate the matching v2 group document instead
of writing a v3 zarr.json. Preserve copying the source zarr.json for v3 stores
and the existing early return when destination metadata already exists.
In `@python/spatialdata-js-util/src/spatialdata_js_util/pyramids.py`:
- Around line 97-100: Update the explicit-level validation in the pyramids logic
to raise WriterCommandError when levels exceeds MAX_LEVELS, rather than silently
clamping it in the return expression. Preserve the existing validation for
values below 1 and keep automatic-level handling unchanged.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tables.py`:
- Around line 198-205: Update _table_dimensions and _convert_table_matrices to
handle table groups without an X child before accessing root["X"]. Skip such
tables or raise the established WriterCommandError so the CLI reports a clear,
supported error instead of propagating KeyError; preserve existing behavior for
tables that contain X.
- Around line 228-240: Before the overwrite removal in the destination-handling
branch, resolve and compare source_path and store_path, rejecting destinations
that are identical to or contain the source store as a parent. Raise
WriterCommandError before shutil.rmtree or shutil.copytree so the source remains
intact, while preserving normal overwrite behavior for unrelated destinations.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tui/screens.py`:
- Around line 732-784: Update the INPUT_ORDER tuple in RecompressImagesScreen to
include the pyramid-related input field identifiers. The current tuple ends with
"workers", but should also include "pyramid-levels", "pyramid-downscale", and
"pyramid-min-size" in that order to ensure form navigation flows through all
input fields before submitting. Add these three identifiers to the end of the
INPUT_ORDER tuple, matching the ids used in the Input widgets defined in the
compose method.
- Around line 907-985: Update TablesToCscScreen._submit to check whether an
explicit dest already exists before creating the task; reject it with an error
notification when it exists and overwrite is false, and require ConfirmScreen
whenever it exists, regardless of overwrite. Preserve the existing in-place
confirmation behavior for dest is None, while allowing RunScreen only for
non-existing explicit destinations.
In `@python/spatialdata-js-util/src/spatialdata_js_util/verify.py`:
- Around line 202-220: Update verify_multiscale_parquet to catch exceptions from
pq.ParquetFile when reading the candidate file, following the error-handling
pattern in verify_morton_parquet. Return checks containing a failed verification
result with the read error details instead of propagating the exception, while
preserving the existing successful metadata validation flow.
In `@python/spatialdata-js-util/tests/test_tables.py`:
- Around line 223-242: Rename test_refresh_refuses_to_write_orphaned_entries to
describe successful reconstruction of the missing points parent, preserving its
current assertions. Add a separate test that creates a genuinely orphaned
consolidated-metadata entry, calls refresh_consolidated_metadata, and asserts
that it raises ValueError, thereby covering the ValueError branch in store.py.
In `@python/spatialdata-js-util/tests/test_tui.py`:
- Around line 216-241: Add the existing HTJ2K availability skip guard to
TestRecompressImages.test_recompresses_losslessly_and_reads_back and both
pyramid tests that press Run, before their TUI execution begins. Reuse the same
guard and availability symbol used by tests/test_recompress.py and
tests/test_zarr_codec.py; leave the validation-only tests unchanged.
In `@python/spatialdata-js-util/tests/test_zarr_codec.py`:
- Around line 46-48: The module-level skipif decorator on
test_zarr_array_round_trips_through_the_codec at line 46-48 blocks both
parametrized codec values based on a single HTJ2K availability check, causing
CODEC_JPEG2K to skip incorrectly when JPEG2K is available without HTJ2K, and to
run incorrectly when only HTJ2K is available. Remove the `@pytest.mark.skipif`
decorator from line 46-48 and add a per-codec availability check inside the
test_zarr_array_round_trips_through_the_codec function that skips the test based
on the codec_name parameter (using a helper that maps CODEC_HTJ2K_OPENJPH to
htj2k_available() and CODEC_JPEG2K to an appropriate JPEG2K availability check).
Apply the same per-codec availability check inside
test_lossless_recompressed_store_reads_back_exactly at lines 147-148 by removing
its module-level skipif decorator and adding equivalent conditional skip logic
based on codec_name.
- Around line 108-114: Update test_selected_backend_passes_the_probe to assert
backend_passes_probe(selected) whenever a backend is selected, rather than
conditionally checking only the BACKEND_IMAGECODECS report entry. Keep the
existing skip behavior when selected is None.
---
Outside diff comments:
In `@scripts/vendor-openjph-for-python.mjs`:
- Around line 31-41: The packaging configuration in pyproject.toml must include
the vendored OpenJPH assets required by encode-plane.mjs and the Python backend:
package the top-level codecs/vendor/*.mjs files,
codecs/vendor/openjph/index.mjs, and codecs/vendor/openjph/wasm/*.wasm files.
Update the relevant package-data inclusion rules without changing the
vendorWasmDir path construction.
---
Nitpick comments:
In
`@python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 113-127: Update readLengthPrefixedJson to accumulate incoming
chunks without repeatedly copying the entire pending buffer: store chunks and
track available bytes, parse the 4-byte length prefix when present, and
concatenate only the chunks needed once the complete declared frame is
available. Preserve support for frames split across chunks and continue yielding
parsed JSON while retaining any trailing bytes for subsequent frames.
In `@python/spatialdata-js-util/src/spatialdata_js_util/points.py`:
- Around line 219-239: The write_multiscale_points_parquet function must record
its applied sort order in the metadata passed to
_write_arrow_table_in_row_groups. Build metadata reflecting the actual sort_keys
list, including gene as the primary key when present, and ensure the resulting
metadata preserves the existing axes, bounds, and levels while adding the
sort-key information.
In `@python/spatialdata-js-util/src/spatialdata_js_util/store.py`:
- Around line 275-283: Remove the redundant path.is_dir() conditional in
read_points_dataframe and call ds.dataset(path, format="parquet").to_table()
once for both files and directories, preserving the existing existence check and
pandas conversion.
- Around line 246-272: Update register_points_elements_in_consolidated_metadata
to persist the modified root document via the existing write_json helper instead
of manually calling json.dumps and write_text, preserving consistent sorted
metadata output. Replace the json.loads/json.dumps deep-copy expression used for
template entries with copy.deepcopy, adding the deepcopy import and applying it
to the existing copy sites in this file.
In `@python/spatialdata-js-util/src/spatialdata_js_util/verify.py`:
- Around line 34-35: Update the _check function’s id parameter and corresponding
VerifyCheck construction to use a non-shadowing name while preserving the same
value. In the line 138 text containing “2–4”, replace the en dash with a
standard hyphen.
- Around line 291-313: Update the verification flow around parquet_path and
verify_morton_parquet so directory datasets accepted by the path check receive
an explicit check when Morton validation is skipped. Preserve Morton
verification for file datasets, and record the skipped or failed outcome with an
id tied to condition_id so all_passed reflects that validation did not run.
In `@python/spatialdata-js-util/tests/test_synthetic_images.py`:
- Around line 31-33: Update
test_mandelbrot_plane_matches_reference_implementation to avoid brittle exact
equality caused by floating-point operation order or dtype differences: compare
the production and reference arrays with a small, explicit tolerance appropriate
for integer iteration counts. Preserve the reference implementation and the
existing size-32 coverage.
In `@python/spatialdata-js-util/tests/test_verify.py`:
- Around line 153-160: Update the existing import in test_verify.py to include
DEFAULT_CONDITIONS, then replace the dynamic __import__ expression in the
conditions tuple with the directly imported symbol while preserving the
canonical and morton filtering.
🪄 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 Plus
Run ID: 4557de99-5a41-4d85-9c03-4dad9c354729
⛔ Files ignored due to path filters (2)
python/spatialdata-experimental-writer/uv.lockis excluded by!**/*.lockpython/spatialdata-js-util/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.cursor/settings.json.github/workflows/test.yml.gitignore.vscode/settings.jsondocs/adr/0002-spatially-aware-vector-loading.mddocs/docs/vis/codec-fixtures.mdxdocs/plans/shapes-nonblocking-tiled-loading.mdpackage.jsonpackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/parquetFooterStats.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/vtableDirectoryResponse.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/zarrextra/README.mdpython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/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/synthetic_cli.pypython/spatialdata-experimental-writer/README.mdpython/spatialdata-experimental-writer/pyproject.tomlpython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.pypython/spatialdata-js-util/README.mdpython/spatialdata-js-util/docs/htj2k-wasm-encode-design.mdpython/spatialdata-js-util/docs/multi-component-codec-findings.mdpython/spatialdata-js-util/pyproject.tomlpython/spatialdata-js-util/scripts/benchmark_points_index.pypython/spatialdata-js-util/scripts/build_htj2k_probe.pypython/spatialdata-js-util/scripts/fixture_writer.pypython/spatialdata-js-util/scripts/generate_codec_fixtures.pypython/spatialdata-js-util/scripts/htj2k_fixtures.pypython/spatialdata-js-util/scripts/mandelbulb_fixtures.pypython/spatialdata-js-util/scripts/provenance.pypython/spatialdata-js-util/scripts/synthetic_images.pypython/spatialdata-js-util/scripts/write_synthetic.pypython/spatialdata-js-util/src/spatialdata_js_util/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/cli.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/chunks.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/htj2k_wasm.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/names.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2cpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjspython/spatialdata-js-util/src/spatialdata_js_util/codecs/zarr_codec.pypython/spatialdata-js-util/src/spatialdata_js_util/errors.pypython/spatialdata-js-util/src/spatialdata_js_util/images.pypython/spatialdata-js-util/src/spatialdata_js_util/index_permutations.pypython/spatialdata-js-util/src/spatialdata_js_util/points.pypython/spatialdata-js-util/src/spatialdata_js_util/provenance.pypython/spatialdata-js-util/src/spatialdata_js_util/pyramids.pypython/spatialdata-js-util/src/spatialdata_js_util/runners.pypython/spatialdata-js-util/src/spatialdata_js_util/store.pypython/spatialdata-js-util/src/spatialdata_js_util/tables.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/app.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/models.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/screens.pypython/spatialdata-js-util/src/spatialdata_js_util/verify.pypython/spatialdata-js-util/tests/conftest.pypython/spatialdata-js-util/tests/test_htj2k_encode.pypython/spatialdata-js-util/tests/test_htj2k_encode_demo.pypython/spatialdata-js-util/tests/test_htj2k_quality.pypython/spatialdata-js-util/tests/test_integration.pypython/spatialdata-js-util/tests/test_points.pypython/spatialdata-js-util/tests/test_pyramids.pypython/spatialdata-js-util/tests/test_recompress.pypython/spatialdata-js-util/tests/test_synthetic_images.pypython/spatialdata-js-util/tests/test_tables.pypython/spatialdata-js-util/tests/test_tui.pypython/spatialdata-js-util/tests/test_verify.pypython/spatialdata-js-util/tests/test_write_synthetic.pypython/spatialdata-js-util/tests/test_writer.pypython/spatialdata-js-util/tests/test_zarr.pypython/spatialdata-js-util/tests/test_zarr_codec.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.ts
💤 Files with no reviewable changes (11)
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py
- python/spatialdata-experimental-writer/README.md
- python/spatialdata-codec-writer/pyproject.toml
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/init.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py
- python/spatialdata-experimental-writer/pyproject.toml
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/init.py
- python/spatialdata-codec-writer/README.md
🛑 Comments failed to post (4)
python/spatialdata-js-util/docs/multi-component-codec-findings.md (1)
133-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the HTJ2K backend description.
These lines state that
imagecodecsis not on the HTJ2K path. The implementation can selectimagecodecsafter its multi-component probe passes, withopenjph-wasmas the fallback. Update this section to match the runtime behavior.🤖 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-js-util/docs/multi-component-codec-findings.md` around lines 133 - 135, Update the HTJ2K backend description in multi-component-codec-findings.md to reflect that the runtime may select imagecodecs when its multi-component probe succeeds, with openjph-wasm used as the fallback; retain imagecodecs’s separate JPEG2000 fixture-path role.python/spatialdata-js-util/scripts/benchmark_points_index.py (1)
27-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd 'index-manifest\.json$' . -x sh -c ' echo "=== $1 ===" jq ".benchmark_scenarios" "$1" ' sh {}Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 169
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== files named benchmark_points_index.py ===" fd 'benchmark_points_index\.py$' . -x sh -c 'echo "$1"; nl -ba "$1" | sed -n "1,140p"' sh {} echo echo "=== references to _load_bounds/_feature_codes ===" rg -n "_load_bounds|_feature_codes|--scenario|benchmark_scenarios|feature_codes" .Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 2748
🏁 Script executed:
#!/bin/bash set -euo pipefail wc -l python/spatialdata-js-util/scripts/benchmark_points_index.py python/spatialdata-js-util/tests/test_points.py python/spatialdata-js-util/src/spatialdata_js_util/index_permutations.py python3 - <<'PY' from pathlib import Path for p in [ Path("python/spatialdata-js-util/scripts/benchmark_points_index.py"), Path("python/spatialdata-js-util/tests/test_points.py"), Path("python/spatialdata-js-util/src/spatialdata_js_util/index_permutations.py"), ]: print(f"=== {p}: {p.stat().st_size} bytes ===") PY echo echo "=== benchmark_points_index.py slices ===" sed -n '1,155p' python/spatialdata-js-util/scripts/benchmark_points_index.py echo echo "=== index_permutations.py benchmark section ===" sed -n '190,220p' python/spatialdata-js-util/src/spatialdata_js_util/index_permutations.py echo echo "=== read-only semantic probe: default selection behavior ===" python3 - <<'PY' import json manifest = { "test": { "feature_key": "feature", "code_column": "code", "scenario_id": "scenario-a", "parquet_path": "a.parquet", }, "benchmark_scenarios": [ {"id": "scenario-a", "feature_codes": [10, 20], "bounds": {"minx": 0}}, {"id": "scenario-b", "feature_codes": [30, 40]}, ], } def current_load_bounds(manifest, scenario_id): scenarios = manifest.get("benchmark_scenarios") or [] if scenario_id is None: return scenarios[0]["bounds"] # mirrored source behavior for no manifest scenario_id raise SystemExit def current_feature_codes(manifest, scenario_id): scenarios = manifest.get("benchmark_scenarios") or [] if scenario_id is None: return None raise SystemExit bounds = current_load_bounds(manifest["test"], manifest["test"].get("scenario_id")) codes = current_feature_codes(manifest["test"], manifest["test"].get("scenario_id")) print({"scenario_id": manifest["test"].get("scenario_id"), "bounds": bounds, "feature_codes": codes}) print({ "scenario_id_in_config": manifest["test"].get("scenario_id"), "scenarios[0].id": manifest["benchmark_scenarios"][0]["id"], "scenario_id matches scenario_a": manifest["test"].get("scenario_id") == "scenario-a", "bound comes_from_scenario_a": bounds == manifest["benchmark_scenarios"][0]["bounds"], "feature_codes_came_from_scenario_a": codes == manifest["benchmark_scenarios"][0]["feature_codes"], }) PYRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 7421
Keep feature filtering on the selected scenario.
--scenariois optional,_load_boundsdefaults to the firstbenchmark_scenariosentry, and_feature_codesreturnsNonewhen no--scenariovalue is present. If the generated manifest definesfeature_codesfor that default scenario, the run applies the scenario bounds without the scenario feature-code filter. Derivefeature_codesfrom the same selected scenario.🤖 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-js-util/scripts/benchmark_points_index.py` around lines 27 - 35, The _feature_codes function returns None when no scenario_id is provided, but _load_bounds defaults to the first benchmark_scenarios entry in this case, creating an inconsistency where scenario bounds are applied without the corresponding feature-code filter. Update _feature_codes to default to the first scenario in the scenarios list when scenario_id is None, matching the defaulting logic of _load_bounds, so that both bounds and feature codes are derived from the same selected scenario.python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs (1)
156-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Flush stdout before exiting, otherwise the parent can hang on a truncated error frame.
When stdout is a pipe, Node writes asynchronously.
process.exit(1)can discard the queued error frame. The Python parent then waits for the 5-byte response header that never arrives.Set
process.exitCodeand let the process end after the stream drains.🛠️ Proposed fix
main().catch((error) => { const message = error instanceof Error ? error.message : String(error); if (workerMode) { writeResponse(1, Buffer.from(message, 'utf8')); - process.exit(1); + process.exitCode = 1; + return; } process.stderr.write(`${message}\n`); - process.exit(1); + process.exitCode = 1; });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.main().catch((error) => { const message = error instanceof Error ? error.message : String(error); if (workerMode) { writeResponse(1, Buffer.from(message, 'utf8')); process.exitCode = 1; return; } process.stderr.write(`${message}\n`); process.exitCode = 1; });🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs` around lines 156 - 164, Update the main().catch error path to avoid calling process.exit(1) immediately after writeResponse or process.stderr.write. Set process.exitCode to 1 instead, allowing stdout to flush fully so the parent receives the complete error frame before the process terminates.python/spatialdata-js-util/src/spatialdata_js_util/verify.py (1)
202-220: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the Parquet read in
verify_multiscale_parquet.Line 211 calls
pq.ParquetFilewithout error handling. A truncated or non-Parquet file raises, so the function propagates an exception instead of returning a failed check.verify_morton_parquetwraps the same call at line 72. The TUI attui/screens.py:1094calls this function and renders the returned checks, so an exception here breaks the report screen instead of showing a failure row.🛡️ Proposed guard
- schema_metadata = pq.ParquetFile(parquet_path).schema_arrow.metadata + try: + schema_metadata = pq.ParquetFile(parquet_path).schema_arrow.metadata + except Exception as exc: + checks.append( + _check("parquet_readable", False, f"failed to read Parquet metadata: {exc}") + ) + return checks📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def verify_multiscale_parquet(path: str | Path) -> list[VerifyCheck]: parquet_path = Path(path) checks: list[VerifyCheck] = [] if not parquet_path.is_file(): return [_check("file_exists", False, f"Parquet file not found: {parquet_path}")] checks.append(_check("file_exists", True, str(parquet_path))) try: schema_metadata = pq.ParquetFile(parquet_path).schema_arrow.metadata except Exception as exc: checks.append( _check("parquet_readable", False, f"failed to read Parquet metadata: {exc}") ) return checks if schema_metadata is None or b"spatialdata_multiscale" not in schema_metadata: checks.append( _check( "multiscale_metadata", False, "missing spatialdata_multiscale schema metadata", ) ) return checks🤖 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-js-util/src/spatialdata_js_util/verify.py` around lines 202 - 220, Update verify_multiscale_parquet to catch exceptions from pq.ParquetFile when reading the candidate file, following the error-handling pattern in verify_morton_parquet. Return checks containing a failed verification result with the read error details instead of propagating the exception, while preserving the existing successful metadata validation flow.
Contributor
Author
|
Worked through the review — 19 fixed, 8 skipped, in 89ef31f. Suite is 140 → 146. Two were worth the whole exercise, and neither was flagged as the severity it turned out to have:
The TUI led straight into it. FixedWrong results, silently:
Errors arriving as tracebacks:
Tests not testing what they claimed:
Also: Done differently
Skipped
|
Merge `spatialdata-codec-writer` (image recompression) and `spatialdata-experimental-writer` (Morton points indexing) into a single distribution suitable for PyPI, and add the two capabilities they were missing: reading our stores back in Python, and CSC table matrices. Zarr reader shim Register `experimental.openjph_htj2k`, `experimental.imagecodecs_htj2k`, and `imagecodecs_jpeg2k` as zarr-python v3 codecs via `zarr.codecs` entry points, so `spatialdata.read_zarr` opens stores this package writes with no import or setup beyond installing the distribution. HTJ2K backend selection `imagecodecs` (native OpenJPH 0.30.1) now serves as a Node-free backend alongside the vendored openjph-wasm build. Measured against the WASM encoder it is byte-exact for lossless at 1/2/3/8 components and within ±1 LSB for lossy, and `level` is the same qstep as our `quality`. Multi-component correctness is not assumed from the library name — the WASM build previously used here silently decoded every component as component 0 (docs/multi-component-codec-findings.md). A backend is admitted only if it decodes a committed multi-component codestream, produced by the WASM encoder, to the exact expected samples. Manifests now record `encoder: "openjph"` with the build in `encoder_backend`, since either backend may have produced the bytes. CSC tables `tables to-csc` converts `X` and named layers from CSR to CSC, so reading one gene is a contiguous range read rather than a scan of every row. Two things this had to get right: AnnData's writer drops SpatialData's element attributes (`region`, `version`, …), which makes `read_zarr` fail, so they are captured and merged back; and anndata >=0.12 exposes `X` as a `None`-keyed layer alias that must not be converted twice. Also: single `spatialdata-js-util` CLI grouped as images/points/tables/ codecs/tui, spatialdata pinned to >=0.8.0, and write-time dependencies moved behind a `[write]` extra so the reader shim installs light. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Point the monorepo docs, ADR 0002, the shapes plan, and the zarrextra README at the consolidated package and its grouped CLI, and write a PyPI-facing README covering the reader shim, the backend probe, and CSC. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three were already unused before the consolidation; they only became visible when the moved files were linted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The TUI predated the consolidation, so it only covered the five Points commands — nothing in it reflected image recompression, CSC tables, or codec backends. Adds `RecompressImagesScreen` and `TablesToCscScreen`, plus table listing and codec-backend info, and regroups the home menu by area (IMAGES/POINTS/TABLES/CODECS). New screens follow the existing form -> confirm -> run -> report flow, so anything that overwrites still stops on a confirmation naming the exact path: converting tables in place, and replacing an existing output store. `VerifyReportScreen` grew a per-command summary rather than the previous rows/output pair, which said nothing useful for a recompression manifest or a CSC report. Tests drive the UI headlessly through real work — filling the forms and pressing the buttons a user would — and assert the store on disk actually changed, including that cancelling a confirm leaves it untouched. The README now leads with the TUI, since it is the easiest entry point and was previously a single buried line in the Points section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Acquisition output is often written at one resolution. A browser then has to fetch full-resolution chunks however far out the user is zoomed, which defeats the image codecs rather than complementing them. Adds `images add-pyramid` and a `--pyramid` option on `images recompress`, so single-resolution input becomes browser-ready in one pass. Levels default to halving until the largest spatial axis fits ~one chunk (1024px); `--pyramid-levels`/`--pyramid-downscale` override that. Images that already have a pyramid are skipped and reported as such unless `--pyramid-force` is passed. Levels go through SpatialData's own parsers rather than hand-written `multiscales` metadata: each level needs a scale *and* a half-pixel translation (s1 by 0.5, s2 by 1.5, ...), and getting those wrong misaligns every level against full resolution. It also means labels are downsampled by the label model rather than averaged, so no level contains an id that identifies no object — verified in the tests. Inside `recompress`, pyramids are staged in a temp store rather than written straight into the destination: `_recompress_image_array` deletes its destination array before streaming the source into it, so reading the new levels from the destination would pull the rug out from under itself. Rebuilding is always copy-then-write. SpatialData refuses to delete the files backing a live element, so there is no in-place mode; `add_pyramids` rejects a destination equal to the source with that explanation. The TUI recompress form carries the same controls, and the report screen shows which rasters were rebuilt or skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Converting the tables of an index-permutations store to CSC left the store unopenable: `spatialdata.read_zarr` failed with GroupNotFoundError at the root, which looked like the table had lost its var names. The table was fine — var names, obs, and every X value were intact and correctly CSC. The fault was entirely in the root's consolidated metadata. `_collect_consolidated_metadata` rebuilds the listing by walking `zarr.json` files on disk. An index-permutations store has `points/<key>/zarr.json` for each element but no `points/zarr.json`, because `_copy_store_shell` skips the points directory and only per-element metadata is written afterwards. Rebuilding therefore dropped the `points` entry while keeping its four children, and zarr rejects the whole store when an entry's parent is missing — not just the affected element. It survived until now because the entry was inherited: the copied root zarr.json already listed `points`, and the index-permutations writer only appended sibling entries to it. Nothing rebuilt that listing from disk until the CSC conversion did. Two fixes, so neither half can reintroduce it: - `_collect_consolidated_metadata` fills in entries for intermediate groups that have no metadata file of their own, and `refresh_consolidated_metadata` now refuses to write a listing containing orphans rather than producing a store that cannot be opened. - `write_index_permutations` writes `points/zarr.json`, so the collection is a real group on disk instead of a bare directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Converting a table written by an older stack left its variable names unreadable in the browser: the JS runtime fell back to `var0..varN`. Every value was present — the encoding had changed underneath it. The conversion re-serialised the whole AnnData, so AnnData 0.13 re-encoded parts of the table it had no reason to touch: - `var/_index` and `obs/_index` became `nullable-string-array` *groups* of `values`/`mask` rather than plain string arrays, so a reader looking for an array found a group and fell back to synthetic names; - every array gained the `sharding_indexed` codec; - `object` columns were normalised to `category`. Now only `X` and any named layers are rewritten, via `write_elem` on the table's zarr group, leaving `obs`/`var`/`uns` byte-identical. What is written pins `auto_shard_zarr_v3` and `allow_write_nullable_strings` off, so a rewritten matrix matches the encodings already in the store. Also drop the table group's own consolidated listing after converting. It caches the encoding of everything below it, and zarr trusts it over the files — X read back as a crc32c checksum failure while the bytes on disk were correct. Tests cover each half: non-matrix nodes are byte-identical across a conversion, a legacy plain-string index is not upgraded, the matrix is not sharded, and a table carrying a stale nested listing still reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Data loss and wrong results first: - `convert_store_tables_to_csc` deleted the source when the destination resolved to it, or to a directory containing it: the overwrite branch rmtree's the destination before copying. Now refused before anything is removed, compared on resolved paths. - The TUI ran straight into that. An existing destination now needs the overwrite box (reported on the form rather than failing mid-run), and replacing one is confirmed the way an in-place rewrite already was. - `resolve_scale_factors` silently clamped an explicit `levels` to `MAX_LEVELS`, building a different pyramid from the one requested and reporting success. It now refuses. - `verify_index_permutations_manifest` reported `all_passed` for a Morton condition backed by a directory dataset, whose ordering it never checked. - `_feature_codes` returned `None` with no scenario selected while `_load_bounds` defaulted to the first scenario, benchmarking that scenario's bounds without its feature filter. Errors that arrived as tracebacks: - A table with no `X` raised `KeyError`. `X` is optional in AnnData, so it is now reported as absent and the layers still convert. - `_recompress_chunks` raised `ArgumentTypeError` from the command body, where argparse no longer catches it. - `verify_multiscale_parquet` propagated a parquet read error instead of reporting the unreadable file as the finding, unlike its Morton sibling. - `LegacyHtj2kCodec` inherits the encode path, so writing through it failed with a generic unsupported-codec error two layers down. The label is registered for reading existing stores only, and now says so. Tests that were not testing what they claimed: - A single HTJ2K gate gated a parametrised HTJ2K/JPEG2K pair, so JPEG2K skipped when only imagecodecs was present and failed when only the WASM backend was. Gated per codec instead. - `test_selected_backend_passes_the_probe` asserted nothing unless the selection happened to be imagecodecs. - `test_refresh_refuses_to_write_orphaned_entries` asserted successful reconstruction, leaving the refusal untested. Renamed, and the guard now has its own test. - Three TUI tests ran a real recompression with no backend guard. Also: docs said imagecodecs was off the HTJ2K path, which the probe-gated backend selection reversed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`build_htj2k_probe.py` imported `codecs.htj2k_wasm` directly and refused to run without Node.js, so removing the Node/WASM encode path would have left the committed probe fixture unreproducible — a frozen artifact gating every backend admission with no way to regenerate it. It now goes through the backend abstraction (`available_backends()`), which makes the WASM backend one option rather than a requirement. Verified by running it with Node off PATH: it produces a valid fixture from imagecodecs alone. That change also fixes what the fixture proved. The old script encoded and round-tripped with the same backend, which only shows self-consistency: a backend mis-ordering components in both directions would have passed. The builder now requires *every* available backend to decode the codestream exactly, so with two installed the fixture is evidence they agree — which is the property that admitting a new backend actually needs. Which backend encoded it is recorded in `multicomponent.json` and reported by `backend_report()`, because a probe result cannot be read correctly without it. The docstring claim that the fixture is "produced by the WASM encoder" would have silently outdated itself on the first regeneration elsewhere. The committed fixture is unchanged: rebuilding reproduces it byte for byte. Worth noting that imagecodecs and openjph-wasm produce byte-identical codestreams for this input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
force-pushed
the
claude/spatialdata-python-package-17c6d7
branch
from
July 31, 2026 14:56
866b061 to
30c9177
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
python/spatialdata-js-util/tests/test_tables.py (1)
156-165: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive table dimensions from
obsandvarwhenXis absent.
_table_dimensions()returns(0, 0)whenXis missing, so this test fixes a misleading manifest for a table whoseobsgroup still holds 30 rows. Useobs/_indexandvar/_indexfallback dimensions here and updaten_obsaccordingly.🤖 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-js-util/tests/test_tables.py` around lines 156 - 165, Update _table_dimensions() to derive row and column counts from obs/_index and var/_index when X is absent, instead of returning (0, 0). Ensure convert_store_tables_to_csc records the fallback row count in the manifest, so the no-X test reports the existing 30 obs rows while keeping X marked absent.python/spatialdata-js-util/src/spatialdata_js_util/tui/app.py (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
BINDINGSasClassVarto satisfy Ruff RUF012.Ruff flags the mutable list default on
BINDINGSas a class attribute. Textual expects this pattern, so runtime behavior is fine, but adding aClassVarannotation removes the lint warning without changing behavior.♻️ Proposed fix
+from typing import ClassVar + - BINDINGS = [("q", "quit", "Quit")] + BINDINGS: ClassVar = [("q", "quit", "Quit")]🤖 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-js-util/src/spatialdata_js_util/tui/app.py` at line 90, Annotate the `BINDINGS` class attribute as `ClassVar` while preserving its existing list value and Textual behavior. Import `ClassVar` from `typing` if needed, and apply the annotation directly to `BINDINGS`.Source: Linters/SAST tools
python/spatialdata-js-util/src/spatialdata_js_util/points.py (1)
149-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep an explicit
sentinel_count=0distinct from an unknown count.Line 149 converts
Noneto0, so an explicitsentinel_count=0and an unknown count take the same path. The auto-detection at lines 151-156 then treats genuine rows whosemorton_code_2dis0as sentinels and isolates them in a separate row group. Run the detection only when the caller passes no count.♻️ Proposed fix
- if sentinel_count is None: - sentinel_count = 0 - if sentinel_count == 0 and MORTON_CODE_2D_COLUMN in table.column_names: - morton_column = table.column(MORTON_CODE_2D_COLUMN).combine_chunks() - for i in range(min(4, table.num_rows)): - if morton_column[i].as_py() != 0: - break - sentinel_count += 1 + if sentinel_count is None: + sentinel_count = 0 + if MORTON_CODE_2D_COLUMN in table.column_names: + morton_column = table.column(MORTON_CODE_2D_COLUMN).combine_chunks() + for i in range(min(4, table.num_rows)): + if morton_column[i].as_py() != 0: + break + sentinel_count += 1🤖 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-js-util/src/spatialdata_js_util/points.py` around lines 149 - 156, Update the sentinel-count handling in the surrounding function so Morton-code auto-detection runs only when the caller-provided sentinel_count is None. Preserve an explicit sentinel_count=0 without converting it into the unknown state or scanning rows, while retaining the existing detection behavior for omitted counts.
🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 156-164: Update the error handling in main() so the workerMode
branch sets process.exitCode to 1 after writeResponse instead of calling
process.exit(1), allowing stdout to flush before Node exits. Preserve the
existing non-worker stderr output and failure status behavior.
In `@python/spatialdata-js-util/src/spatialdata_js_util/store.py`:
- Around line 247-273: Update register_points_elements_in_consolidated_metadata
to ensure the consolidated metadata includes the "points" parent entry before
adding points/{key} entries. Create that parent entry using the same
_implicit_group representation used by _collect_consolidated_metadata,
preserving existing metadata when already present, then write the updated
document.
- Around line 61-85: Update _collect_consolidated_metadata to discover and
register .zgroup files alongside zarr.json entries when rebuilding consolidated
metadata. Read each v2 group’s metadata into the mapping and preserve its own
zarr_format value, while continuing to synthesize missing intermediate parents
with _implicit_group using the appropriate format.
---
Nitpick comments:
In `@python/spatialdata-js-util/src/spatialdata_js_util/points.py`:
- Around line 149-156: Update the sentinel-count handling in the surrounding
function so Morton-code auto-detection runs only when the caller-provided
sentinel_count is None. Preserve an explicit sentinel_count=0 without converting
it into the unknown state or scanning rows, while retaining the existing
detection behavior for omitted counts.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tui/app.py`:
- Line 90: Annotate the `BINDINGS` class attribute as `ClassVar` while
preserving its existing list value and Textual behavior. Import `ClassVar` from
`typing` if needed, and apply the annotation directly to `BINDINGS`.
In `@python/spatialdata-js-util/tests/test_tables.py`:
- Around line 156-165: Update _table_dimensions() to derive row and column
counts from obs/_index and var/_index when X is absent, instead of returning (0,
0). Ensure convert_store_tables_to_csc records the fallback row count in the
manifest, so the no-X test reports the existing 30 obs rows while keeping X
marked absent.
🪄 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 Plus
Run ID: 23ad353e-5738-4b89-b74b-eab43eccf06a
⛔ Files ignored due to path filters (2)
python/spatialdata-experimental-writer/uv.lockis excluded by!**/*.lockpython/spatialdata-js-util/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.cursor/settings.json.github/workflows/test.yml.gitignore.vscode/settings.jsondocs/adr/0002-spatially-aware-vector-loading.mddocs/docs/vis/codec-fixtures.mdxdocs/plans/shapes-nonblocking-tiled-loading.mdpackage.jsonpackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/parquetFooterStats.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/vtableDirectoryResponse.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/zarrextra/README.mdpython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/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/synthetic_cli.pypython/spatialdata-experimental-writer/README.mdpython/spatialdata-experimental-writer/pyproject.tomlpython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.pypython/spatialdata-js-util/README.mdpython/spatialdata-js-util/docs/htj2k-wasm-encode-design.mdpython/spatialdata-js-util/docs/multi-component-codec-findings.mdpython/spatialdata-js-util/pyproject.tomlpython/spatialdata-js-util/scripts/benchmark_points_index.pypython/spatialdata-js-util/scripts/build_htj2k_probe.pypython/spatialdata-js-util/scripts/fixture_writer.pypython/spatialdata-js-util/scripts/generate_codec_fixtures.pypython/spatialdata-js-util/scripts/htj2k_fixtures.pypython/spatialdata-js-util/scripts/mandelbulb_fixtures.pypython/spatialdata-js-util/scripts/provenance.pypython/spatialdata-js-util/scripts/synthetic_images.pypython/spatialdata-js-util/scripts/write_synthetic.pypython/spatialdata-js-util/src/spatialdata_js_util/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/cli.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/chunks.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/htj2k_wasm.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/names.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2cpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.jsonpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjspython/spatialdata-js-util/src/spatialdata_js_util/codecs/zarr_codec.pypython/spatialdata-js-util/src/spatialdata_js_util/errors.pypython/spatialdata-js-util/src/spatialdata_js_util/images.pypython/spatialdata-js-util/src/spatialdata_js_util/index_permutations.pypython/spatialdata-js-util/src/spatialdata_js_util/points.pypython/spatialdata-js-util/src/spatialdata_js_util/provenance.pypython/spatialdata-js-util/src/spatialdata_js_util/pyramids.pypython/spatialdata-js-util/src/spatialdata_js_util/runners.pypython/spatialdata-js-util/src/spatialdata_js_util/store.pypython/spatialdata-js-util/src/spatialdata_js_util/tables.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/app.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/models.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/screens.pypython/spatialdata-js-util/src/spatialdata_js_util/verify.pypython/spatialdata-js-util/tests/conftest.pypython/spatialdata-js-util/tests/test_htj2k_encode.pypython/spatialdata-js-util/tests/test_htj2k_encode_demo.pypython/spatialdata-js-util/tests/test_htj2k_quality.pypython/spatialdata-js-util/tests/test_integration.pypython/spatialdata-js-util/tests/test_points.pypython/spatialdata-js-util/tests/test_pyramids.pypython/spatialdata-js-util/tests/test_recompress.pypython/spatialdata-js-util/tests/test_synthetic_images.pypython/spatialdata-js-util/tests/test_tables.pypython/spatialdata-js-util/tests/test_tui.pypython/spatialdata-js-util/tests/test_verify.pypython/spatialdata-js-util/tests/test_write_synthetic.pypython/spatialdata-js-util/tests/test_writer.pypython/spatialdata-js-util/tests/test_zarr.pypython/spatialdata-js-util/tests/test_zarr_codec.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.ts
💤 Files with no reviewable changes (11)
- python/spatialdata-codec-writer/README.md
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/init.py
- python/spatialdata-experimental-writer/pyproject.toml
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/init.py
- python/spatialdata-experimental-writer/README.md
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py
- python/spatialdata-codec-writer/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (42)
- docs/adr/0002-spatially-aware-vector-loading.md
- python/spatialdata-js-util/scripts/mandelbulb_fixtures.py
- packages/zarrextra/README.md
- python/spatialdata-js-util/src/spatialdata_js_util/errors.py
- python/spatialdata-js-util/src/spatialdata_js_util/tui/models.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2c
- python/spatialdata-js-util/scripts/fixture_writer.py
- python/spatialdata-js-util/tests/test_write_synthetic.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/init.py
- python/spatialdata-js-util/scripts/write_synthetic.py
- .gitignore
- python/spatialdata-js-util/docs/htj2k-wasm-encode-design.md
- .cursor/settings.json
- python/spatialdata-js-util/scripts/generate_codec_fixtures.py
- scripts/encode-htj2k-plane.mjs
- .vscode/settings.json
- python/spatialdata-js-util/tests/test_zarr.py
- python/spatialdata-js-util/tests/conftest.py
- scripts/vendor-openjph-for-python.mjs
- python/spatialdata-js-util/tests/test_htj2k_encode_demo.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/names.py
- .github/workflows/test.yml
- python/spatialdata-js-util/tests/test_tui.py
- python/spatialdata-js-util/src/spatialdata_js_util/runners.py
- python/spatialdata-js-util/tests/test_synthetic_images.py
- python/spatialdata-js-util/tests/test_htj2k_encode.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npy
- docs/plans/shapes-nonblocking-tiled-loading.md
- python/spatialdata-js-util/scripts/htj2k_fixtures.py
- python/spatialdata-js-util/tests/test_verify.py
- python/spatialdata-js-util/src/spatialdata_js_util/provenance.py
- package.json
- python/spatialdata-js-util/tests/test_points.py
- python/spatialdata-js-util/tests/test_htj2k_quality.py
- python/spatialdata-js-util/pyproject.toml
- python/spatialdata-js-util/tests/test_writer.py
- python/spatialdata-js-util/scripts/benchmark_points_index.py
- python/spatialdata-js-util/tests/test_integration.py
- python/spatialdata-js-util/src/spatialdata_js_util/init.py
- python/spatialdata-js-util/src/spatialdata_js_util/images.py
- python/spatialdata-js-util/tests/test_recompress.py
- python/spatialdata-js-util/src/spatialdata_js_util/tui/screens.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
python/spatialdata-js-util/tests/test_tables.py (1)
156-165: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive table dimensions from
obsandvarwhenXis absent.
_table_dimensions()returns(0, 0)whenXis missing, so this test fixes a misleading manifest for a table whoseobsgroup still holds 30 rows. Useobs/_indexandvar/_indexfallback dimensions here and updaten_obsaccordingly.🤖 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-js-util/tests/test_tables.py` around lines 156 - 165, Update _table_dimensions() to derive row and column counts from obs/_index and var/_index when X is absent, instead of returning (0, 0). Ensure convert_store_tables_to_csc records the fallback row count in the manifest, so the no-X test reports the existing 30 obs rows while keeping X marked absent.python/spatialdata-js-util/src/spatialdata_js_util/tui/app.py (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
BINDINGSasClassVarto satisfy Ruff RUF012.Ruff flags the mutable list default on
BINDINGSas a class attribute. Textual expects this pattern, so runtime behavior is fine, but adding aClassVarannotation removes the lint warning without changing behavior.♻️ Proposed fix
+from typing import ClassVar + - BINDINGS = [("q", "quit", "Quit")] + BINDINGS: ClassVar = [("q", "quit", "Quit")]🤖 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-js-util/src/spatialdata_js_util/tui/app.py` at line 90, Annotate the `BINDINGS` class attribute as `ClassVar` while preserving its existing list value and Textual behavior. Import `ClassVar` from `typing` if needed, and apply the annotation directly to `BINDINGS`.Source: Linters/SAST tools
python/spatialdata-js-util/src/spatialdata_js_util/points.py (1)
149-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep an explicit
sentinel_count=0distinct from an unknown count.Line 149 converts
Noneto0, so an explicitsentinel_count=0and an unknown count take the same path. The auto-detection at lines 151-156 then treats genuine rows whosemorton_code_2dis0as sentinels and isolates them in a separate row group. Run the detection only when the caller passes no count.♻️ Proposed fix
- if sentinel_count is None: - sentinel_count = 0 - if sentinel_count == 0 and MORTON_CODE_2D_COLUMN in table.column_names: - morton_column = table.column(MORTON_CODE_2D_COLUMN).combine_chunks() - for i in range(min(4, table.num_rows)): - if morton_column[i].as_py() != 0: - break - sentinel_count += 1 + if sentinel_count is None: + sentinel_count = 0 + if MORTON_CODE_2D_COLUMN in table.column_names: + morton_column = table.column(MORTON_CODE_2D_COLUMN).combine_chunks() + for i in range(min(4, table.num_rows)): + if morton_column[i].as_py() != 0: + break + sentinel_count += 1🤖 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-js-util/src/spatialdata_js_util/points.py` around lines 149 - 156, Update the sentinel-count handling in the surrounding function so Morton-code auto-detection runs only when the caller-provided sentinel_count is None. Preserve an explicit sentinel_count=0 without converting it into the unknown state or scanning rows, while retaining the existing detection behavior for omitted counts.
🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs`:
- Around line 156-164: Update the error handling in main() so the workerMode
branch sets process.exitCode to 1 after writeResponse instead of calling
process.exit(1), allowing stdout to flush before Node exits. Preserve the
existing non-worker stderr output and failure status behavior.
In `@python/spatialdata-js-util/src/spatialdata_js_util/store.py`:
- Around line 247-273: Update register_points_elements_in_consolidated_metadata
to ensure the consolidated metadata includes the "points" parent entry before
adding points/{key} entries. Create that parent entry using the same
_implicit_group representation used by _collect_consolidated_metadata,
preserving existing metadata when already present, then write the updated
document.
- Around line 61-85: Update _collect_consolidated_metadata to discover and
register .zgroup files alongside zarr.json entries when rebuilding consolidated
metadata. Read each v2 group’s metadata into the mapping and preserve its own
zarr_format value, while continuing to synthesize missing intermediate parents
with _implicit_group using the appropriate format.
---
Nitpick comments:
In `@python/spatialdata-js-util/src/spatialdata_js_util/points.py`:
- Around line 149-156: Update the sentinel-count handling in the surrounding
function so Morton-code auto-detection runs only when the caller-provided
sentinel_count is None. Preserve an explicit sentinel_count=0 without converting
it into the unknown state or scanning rows, while retaining the existing
detection behavior for omitted counts.
In `@python/spatialdata-js-util/src/spatialdata_js_util/tui/app.py`:
- Line 90: Annotate the `BINDINGS` class attribute as `ClassVar` while
preserving its existing list value and Textual behavior. Import `ClassVar` from
`typing` if needed, and apply the annotation directly to `BINDINGS`.
In `@python/spatialdata-js-util/tests/test_tables.py`:
- Around line 156-165: Update _table_dimensions() to derive row and column
counts from obs/_index and var/_index when X is absent, instead of returning (0,
0). Ensure convert_store_tables_to_csc records the fallback row count in the
manifest, so the no-X test reports the existing 30 obs rows while keeping X
marked absent.
🪄 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 Plus
Run ID: 23ad353e-5738-4b89-b74b-eab43eccf06a
⛔ Files ignored due to path filters (2)
python/spatialdata-experimental-writer/uv.lockis excluded by!**/*.lockpython/spatialdata-js-util/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.cursor/settings.json.github/workflows/test.yml.gitignore.vscode/settings.jsondocs/adr/0002-spatially-aware-vector-loading.mddocs/docs/vis/codec-fixtures.mdxdocs/plans/shapes-nonblocking-tiled-loading.mdpackage.jsonpackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/parquetFooterStats.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/vtableDirectoryResponse.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/zarrextra/README.mdpython/spatialdata-codec-writer/README.mdpython/spatialdata-codec-writer/pyproject.tomlpython/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/synthetic_cli.pypython/spatialdata-experimental-writer/README.mdpython/spatialdata-experimental-writer/pyproject.tomlpython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.pypython/spatialdata-js-util/README.mdpython/spatialdata-js-util/docs/htj2k-wasm-encode-design.mdpython/spatialdata-js-util/docs/multi-component-codec-findings.mdpython/spatialdata-js-util/pyproject.tomlpython/spatialdata-js-util/scripts/benchmark_points_index.pypython/spatialdata-js-util/scripts/build_htj2k_probe.pypython/spatialdata-js-util/scripts/fixture_writer.pypython/spatialdata-js-util/scripts/generate_codec_fixtures.pypython/spatialdata-js-util/scripts/htj2k_fixtures.pypython/spatialdata-js-util/scripts/mandelbulb_fixtures.pypython/spatialdata-js-util/scripts/provenance.pypython/spatialdata-js-util/scripts/synthetic_images.pypython/spatialdata-js-util/scripts/write_synthetic.pypython/spatialdata-js-util/src/spatialdata_js_util/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/cli.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/backends.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/chunks.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/encoding.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/htj2k_wasm.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/names.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2cpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.jsonpython/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjspython/spatialdata-js-util/src/spatialdata_js_util/codecs/zarr_codec.pypython/spatialdata-js-util/src/spatialdata_js_util/errors.pypython/spatialdata-js-util/src/spatialdata_js_util/images.pypython/spatialdata-js-util/src/spatialdata_js_util/index_permutations.pypython/spatialdata-js-util/src/spatialdata_js_util/points.pypython/spatialdata-js-util/src/spatialdata_js_util/provenance.pypython/spatialdata-js-util/src/spatialdata_js_util/pyramids.pypython/spatialdata-js-util/src/spatialdata_js_util/runners.pypython/spatialdata-js-util/src/spatialdata_js_util/store.pypython/spatialdata-js-util/src/spatialdata_js_util/tables.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/__init__.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/app.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/models.pypython/spatialdata-js-util/src/spatialdata_js_util/tui/screens.pypython/spatialdata-js-util/src/spatialdata_js_util/verify.pypython/spatialdata-js-util/tests/conftest.pypython/spatialdata-js-util/tests/test_htj2k_encode.pypython/spatialdata-js-util/tests/test_htj2k_encode_demo.pypython/spatialdata-js-util/tests/test_htj2k_quality.pypython/spatialdata-js-util/tests/test_integration.pypython/spatialdata-js-util/tests/test_points.pypython/spatialdata-js-util/tests/test_pyramids.pypython/spatialdata-js-util/tests/test_recompress.pypython/spatialdata-js-util/tests/test_synthetic_images.pypython/spatialdata-js-util/tests/test_tables.pypython/spatialdata-js-util/tests/test_tui.pypython/spatialdata-js-util/tests/test_verify.pypython/spatialdata-js-util/tests/test_write_synthetic.pypython/spatialdata-js-util/tests/test_writer.pypython/spatialdata-js-util/tests/test_zarr.pypython/spatialdata-js-util/tests/test_zarr_codec.pyscripts/encode-htj2k-plane.mjsscripts/vendor-openjph-for-python.mjstests/integration/codecFixtures.test.ts
💤 Files with no reviewable changes (11)
- python/spatialdata-codec-writer/README.md
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/codecs.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/init.py
- python/spatialdata-experimental-writer/pyproject.toml
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/synthetic_cli.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/cli.py
- python/spatialdata-codec-writer/src/spatialdata_codec_writer/init.py
- python/spatialdata-experimental-writer/README.md
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py
- python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py
- python/spatialdata-codec-writer/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (42)
- docs/adr/0002-spatially-aware-vector-loading.md
- python/spatialdata-js-util/scripts/mandelbulb_fixtures.py
- packages/zarrextra/README.md
- python/spatialdata-js-util/src/spatialdata_js_util/errors.py
- python/spatialdata-js-util/src/spatialdata_js_util/tui/models.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.j2c
- python/spatialdata-js-util/scripts/fixture_writer.py
- python/spatialdata-js-util/tests/test_write_synthetic.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/init.py
- python/spatialdata-js-util/scripts/write_synthetic.py
- .gitignore
- python/spatialdata-js-util/docs/htj2k-wasm-encode-design.md
- .cursor/settings.json
- python/spatialdata-js-util/scripts/generate_codec_fixtures.py
- scripts/encode-htj2k-plane.mjs
- .vscode/settings.json
- python/spatialdata-js-util/tests/test_zarr.py
- python/spatialdata-js-util/tests/conftest.py
- scripts/vendor-openjph-for-python.mjs
- python/spatialdata-js-util/tests/test_htj2k_encode_demo.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/names.py
- .github/workflows/test.yml
- python/spatialdata-js-util/tests/test_tui.py
- python/spatialdata-js-util/src/spatialdata_js_util/runners.py
- python/spatialdata-js-util/tests/test_synthetic_images.py
- python/spatialdata-js-util/tests/test_htj2k_encode.py
- python/spatialdata-js-util/src/spatialdata_js_util/codecs/probe/multicomponent.npy
- docs/plans/shapes-nonblocking-tiled-loading.md
- python/spatialdata-js-util/scripts/htj2k_fixtures.py
- python/spatialdata-js-util/tests/test_verify.py
- python/spatialdata-js-util/src/spatialdata_js_util/provenance.py
- package.json
- python/spatialdata-js-util/tests/test_points.py
- python/spatialdata-js-util/tests/test_htj2k_quality.py
- python/spatialdata-js-util/pyproject.toml
- python/spatialdata-js-util/tests/test_writer.py
- python/spatialdata-js-util/scripts/benchmark_points_index.py
- python/spatialdata-js-util/tests/test_integration.py
- python/spatialdata-js-util/src/spatialdata_js_util/init.py
- python/spatialdata-js-util/src/spatialdata_js_util/images.py
- python/spatialdata-js-util/tests/test_recompress.py
- python/spatialdata-js-util/src/spatialdata_js_util/tui/screens.py
🛑 Comments failed to post (1)
python/spatialdata-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs (1)
156-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Set
process.exitCodeinstead of callingprocess.exit()after writing to stdout.When stdout is a pipe, writes are asynchronous.
process.exit(1)can terminate the process before the queued header and payload are flushed. The Python parent then reads a truncated frame or blocks waiting for the remaining bytes.Set the exit code and let Node exit after the stream drains.
🐛 Proposed fix
main().catch((error) => { const message = error instanceof Error ? error.message : String(error); if (workerMode) { writeResponse(1, Buffer.from(message, 'utf8')); - process.exit(1); + process.exitCode = 1; + return; } process.stderr.write(`${message}\n`); - process.exit(1); + process.exitCode = 1; });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.main().catch((error) => { const message = error instanceof Error ? error.message : String(error); if (workerMode) { writeResponse(1, Buffer.from(message, 'utf8')); process.exitCode = 1; return; } process.stderr.write(`${message}\n`); process.exitCode = 1; });🤖 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-js-util/src/spatialdata_js_util/codecs/vendor/encode-plane.mjs` around lines 156 - 164, Update the error handling in main() so the workerMode branch sets process.exitCode to 1 after writeResponse instead of calling process.exit(1), allowing stdout to flush before Node exits. Preserve the existing non-worker stderr output and failure status behavior.
`register_points_elements_in_consolidated_metadata` adds `points/<key>` to the root listing but never checked that a `points` entry exists. Where the source store has none — the broken-writer case this module exists to handle — the result is an orphaned entry, and zarr rejects the *whole* store rather than the one element. Reproduced: `GroupNotFoundError` at the root, with every element gone, not just the new sibling. This is the failure `refresh_consolidated_metadata` already guards against. It was reachable here because this function edits the map directly instead of rebuilding it from disk, so it never went through that guard. The parent-filling loop is now shared with `_collect_consolidated_metadata`, and this function runs the same orphan check before writing, so neither writer of the listing can produce one. 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.
Consolidates the two Python packages into one publishable
spatialdata-js-util, and adds CSC conversion, multiscale pyramids and a TUI.Why one package
spatialdata-codec-writerandspatialdata-experimental-writerwere split by which script needed them, not by what they do, and neither could be installed by anyone wanting to read one of our stores. The merged package registers our HTJ2K codec through zarr'szarr.codecsentry points, so a plainpip install spatialdata-js-utilis enough to open a store inspatialdata— verified from a bare venv with no Node, nospatialdataand noanndatapresent.utilrather thanwriterbecause reading is now half the point, and the name leaves room for the notebook-side things we have talked about.Decode backends
imagecodecsis now the preferred backend, with the Node/WASM encoder as the fallback. This matters more than it sounds: it removes Node from the install path entirely for read-only use.That preference is not taken on trust. Backend selection is gated by a committed probe — a 423-byte multi-component codestream produced by the WASM encoder, decoded and compared exactly. WASM is trusted on availability because it produced the fixture; everything else has to prove itself at import time. This was worth building because the failure it guards against is silent: an openjph build that mishandles multi-component data returns plausible pixels, not an error.
Findings from the evaluation are in
docs/multi-component-codec-findings.md. Short version:imagecodecs' openjph 0.30.1 is multi-component-correct and honoursqstep, so we lose no quality control by preferring it.CSC conversion
convert_store_tables_to_cscrewritesXand named layers in place.Deliberately scoped to the matrices — an earlier version round-tripped the whole
AnnData, which is how you lose things. Underanndata0.13.2 that re-serialisation silently changed_indexfrom a plain string array tonullable-string-arrayand shardedX; the store still opened in Python and still looked right there, while the JS reader lost every var name. Write settings are pinned for the same reason. The frontend PR stacked on this one handles the encoding on the reading side, which is the real fix — this just stops us causing it.Pyramids
add_pyramidsgenerates missing multiscale levels throughspatialdata's own model parsers, so the half-pixel translations and label-safe downsampling come out right rather than being reimplemented. Auto-halves until the largest spatial axis is under 1024, capped at 12 levels.Also
pointscollection had nozarr.jsonof its own became unopenable entirely after a refresh — one orphaned entry rejects the whole store, not just that element. Now reconstructed, with an explicit orphan check that raises rather than writing a listing that cannot be read.spatialdata-js-util tui), covering recompression, points indexing, CSC conversion, pyramids and codec info.Testing
140 passed, 1 skipped. The TS changes here are call-site renames forced by the package move — no behaviour change, and the full JS suite passes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation