Add label and shape capabilities#91
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds an Arrow-native ChangesOME-Arrow Shapes feature
README schema image generator
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d33bs
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/src/_static/generate_schema_image.py (1)
14-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent font type annotations.
_font()is typed to returnImageFont.FreeTypeFont, but the fallback path returnsImageFont.load_default(...), whose signature isFreeTypeFont | ImageFont— not guaranteed to beFreeTypeFont. Meanwhile_text_center()'sfontparameter is typed as the (unrelated) baseImageFont.ImageFontclass. These annotations don't align with each other or with the actual return type, which would surface as type-checker errors if mypy/pyright is run on this file.♻️ Suggested fix
-def _font(size: int, *, bold: bool = False) -> ImageFont.FreeTypeFont: +def _font(size: int, *, bold: bool = False) -> ImageFont.FreeTypeFont | ImageFont.ImageFont:Also applies to: 29-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/_static/generate_schema_image.py` around lines 14 - 26, The font annotations in _font and _text_center are inconsistent with the actual return types, so align them to match Pillow’s types. Update _font so its return annotation covers both the truetype path and the load_default fallback, and make _text_center accept the same font type that _font returns. Use the unique symbols _font and _text_center to keep the signatures consistent and avoid mypy/pyright errors.tests/test_shapes.py (2)
93-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSecond branch doesn't test the intended "missing column" path.
The row omits
object_id, but sinceshape_schemaalways includesobject_idas a schema field,pa.Table.from_pylistfills it with null rather than omitting the column. Sovalidate_shape_tablenever hits the"object_id" not in table.column_names"branch here — it instead fails on the later null-check ("object_id values must not be null"), which happens to also match the"object_id"regex. The test still passes, but it isn't exercising the code path its docstring implies.♻️ Suggested fix to actually cover the missing-column branch
- table = make_shape_table( - [{"geometry": [0.0, 1.0]}], - geometry_encoding="geoarrow.point", - validate=False, - ) - - with pytest.raises(ValueError, match="object_id"): - validate_shape_table(table) + table = make_shape_table( + [{"geometry": [0.0, 1.0]}], + geometry_encoding="geoarrow.point", + validate=False, + ).drop_columns(["object_id"]) + + with pytest.raises(ValueError, match="must contain an object_id column"): + validate_shape_table(table)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_shapes.py` around lines 93 - 107, The second assertion in test_validate_shape_table currently exercises the null-value path instead of the missing-column branch in validate_shape_table. Update the test setup so the table truly lacks object_id in table.column_names, using make_shape_table or a direct pyarrow table definition that omits the column, and keep the assertion targeting the missing-column error path in validate_shape_table.
139-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo negative-path coverage for
validate_relationship_table.Only the happy path is tested; the missing-column, null-value, and unsupported-
relationship_typebranches shown invalidate_relationship_table(src/ome_arrow/shapes.py:467-497) are untested here. Consider adding cases similar to the shape-table validation tests. As per relevant code snippets,validate_relationship_tableraisesValueErrorfor missing schema metadata, missing/nullparent_id/child_id/relationship_type, and unsupported relationship types — none of these branches currently have test coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_shapes.py` around lines 139 - 186, Add negative-path tests for validate_relationship_table in the existing relationship-table test module: keep the current happy-path test, and add focused cases that exercise the missing schema metadata, missing or null parent_id/child_id/relationship_type fields, and unsupported relationship_type branches. Use make_relationship_table and validate_relationship_table to build each scenario, and assert the expected ValueError is raised for each invalid input, mirroring the style of the shape-table validation tests.tests/test_shapes_performance.py (1)
34-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded wall-clock thresholds risk flaky CI failures.
All three tests assert absolute elapsed-time bounds (
< 1.0,< 0.25,< 1.5,< 0.5) with a single untimed run. On shared/loaded CI runners these thresholds can be exceeded without any actual regression, causing intermittent, hard-to-reproduce failures that aren't tied to code changes.Consider one of:
- Using a proper benchmarking tool (e.g.
pytest-benchmark) that accounts for variance/warm-up.- Running multiple iterations and asserting on a median/percentile rather than a single timing.
- Marking these as an opt-in/slow test group excluded from default CI runs (matching the separate
benchmarks/script already added for this purpose).- Using more generous thresholds with headroom for CI variance if these must stay in the default suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_shapes_performance.py` around lines 34 - 81, The performance tests in test_shapes_performance.py use single-run wall-clock cutoffs that can be flaky on busy CI runners. Update the checks around make_shape_table, OMEArrowShapes.for_image, and write_shape_parquet/read_shape_parquet to avoid strict absolute timing assertions; prefer a benchmark-style approach, multiple iterations with a median/percentile, or move them behind a slow/opt-in test marker. If they remain in the default suite, relax the thresholds with enough CI headroom.src/ome_arrow/shapes.py (1)
16-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
SUPPORTED_GEOMETRY_ENCODINGSfromGeometryEncodingto avoid drift.The frozenset duplicates the
Literalvalues verbatim. If a new encoding is added to one but not the other, the type hint and the runtime allow-list silently diverge.♻️ Proposed fix
+from typing import get_args + GeometryEncoding = Literal[ "geoarrow.point", "geoarrow.linestring", "geoarrow.polygon", "geoarrow.multipolygon", "ome.mesh3d", "ome.labelmask", "ome.pointcloud", "ome.boundingbox", ] -SUPPORTED_GEOMETRY_ENCODINGS: frozenset[str] = frozenset( - { - "geoarrow.point", - "geoarrow.linestring", - "geoarrow.polygon", - "geoarrow.multipolygon", - "ome.mesh3d", - "ome.labelmask", - "ome.pointcloud", - "ome.boundingbox", - } -) +SUPPORTED_GEOMETRY_ENCODINGS: frozenset[str] = frozenset(get_args(GeometryEncoding))🤖 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 `@src/ome_arrow/shapes.py` around lines 16 - 38, Keep GeometryEncoding and SUPPORTED_GEOMETRY_ENCODINGS in sync by deriving the runtime allow-list from the GeometryEncoding literal instead of duplicating the values in both places. Update the symbols GeometryEncoding and SUPPORTED_GEOMETRY_ENCODINGS in shapes.py so there is a single source of truth for the supported geometry strings, preventing the type hint and allow-list from drifting apart when new encodings are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/shapes.md`:
- Around line 87-101: The canonical column table in shapes.md incorrectly lists
`class` and `confidence` as reserved schema columns, which conflicts with
`make_shape_table` treating them as non-core measurements. Update the
canonical-columns list near the shape schema description to keep only the true
core fields (`object_id`, `image_id`, `label_image_id`, `label_value`,
`geometry`, `centroid`, `bbox`) and move `class` and `confidence` into the
measurements/examples discussion so the schema contract stays aligned with
`make_shape_table`.
In `@src/ome_arrow/shapes.py`:
- Around line 76-80: The _coordinate_type() helper currently ignores the
dimensions argument, so shape_schema() and validate_shape_table() accept
arbitrary coordinate lengths for geometry, centroid, and bbox. Update
_coordinate_type() to enforce the expected arity using dimensions (or add an
explicit per-row length validation if mixed dimensionality is intended), and
make sure shape_schema() and validate_shape_table() rely on that fixed shape.
Also adjust the related schema expectation in tests/test_shapes.py to match the
new coordinate type.
- Around line 253-303: The custom geometry column is still being treated as a
measurement field, which creates duplicate columns and breaks validation. Update
make_shape_table to pass geometry_column into _infer_measurement_fields, and
change _infer_measurement_fields to exclude that column name along with
DEFAULT_SHAPE_COLUMNS. Use the existing symbols make_shape_table and
_infer_measurement_fields to keep the geometry column reserved and ensure
validate_shape_table can resolve it correctly.
---
Nitpick comments:
In `@docs/src/_static/generate_schema_image.py`:
- Around line 14-26: The font annotations in _font and _text_center are
inconsistent with the actual return types, so align them to match Pillow’s
types. Update _font so its return annotation covers both the truetype path and
the load_default fallback, and make _text_center accept the same font type that
_font returns. Use the unique symbols _font and _text_center to keep the
signatures consistent and avoid mypy/pyright errors.
In `@src/ome_arrow/shapes.py`:
- Around line 16-38: Keep GeometryEncoding and SUPPORTED_GEOMETRY_ENCODINGS in
sync by deriving the runtime allow-list from the GeometryEncoding literal
instead of duplicating the values in both places. Update the symbols
GeometryEncoding and SUPPORTED_GEOMETRY_ENCODINGS in shapes.py so there is a
single source of truth for the supported geometry strings, preventing the type
hint and allow-list from drifting apart when new encodings are added.
In `@tests/test_shapes_performance.py`:
- Around line 34-81: The performance tests in test_shapes_performance.py use
single-run wall-clock cutoffs that can be flaky on busy CI runners. Update the
checks around make_shape_table, OMEArrowShapes.for_image, and
write_shape_parquet/read_shape_parquet to avoid strict absolute timing
assertions; prefer a benchmark-style approach, multiple iterations with a
median/percentile, or move them behind a slow/opt-in test marker. If they remain
in the default suite, relax the thresholds with enough CI headroom.
In `@tests/test_shapes.py`:
- Around line 93-107: The second assertion in test_validate_shape_table
currently exercises the null-value path instead of the missing-column branch in
validate_shape_table. Update the test setup so the table truly lacks object_id
in table.column_names, using make_shape_table or a direct pyarrow table
definition that omits the column, and keep the assertion targeting the
missing-column error path in validate_shape_table.
- Around line 139-186: Add negative-path tests for validate_relationship_table
in the existing relationship-table test module: keep the current happy-path
test, and add focused cases that exercise the missing schema metadata, missing
or null parent_id/child_id/relationship_type fields, and unsupported
relationship_type branches. Use make_relationship_table and
validate_relationship_table to build each scenario, and assert the expected
ValueError is raised for each invalid input, mirroring the style of the
shape-table validation tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0e3c69d-85a8-4da4-9a1d-982ca80e00fc
⛔ Files ignored due to path filters (1)
docs/src/_static/various_ome_arrow_schema.pngis excluded by!**/*.png
📒 Files selected for processing (10)
README.mdbenchmarks/benchmark_shapes_parquet.pydocs/src/_static/generate_schema_image.pydocs/src/index.mddocs/src/python-api.mddocs/src/shapes.mdsrc/ome_arrow/__init__.pysrc/ome_arrow/shapes.pytests/test_shapes.pytests/test_shapes_performance.py
| The canonical columns are: | ||
|
|
||
| | Column | Purpose | | ||
| | ---------------- | --------------------------------------- | | ||
| | `object_id` | stable object identifier | | ||
| | `image_id` | source image identifier | | ||
| | `label_image_id` | source label image identifier | | ||
| | `label_value` | integer label value in the label raster | | ||
| | `geometry` | one logical geometry value | | ||
| | `centroid` | coordinate vector for object center | | ||
| | `bbox` | min/max coordinate bounds | | ||
| | `class` | object class or annotation category | | ||
| | `confidence` | detection or classification confidence | | ||
|
|
||
| Any additional columns are measurements. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move class and confidence out of the canonical-column list.
make_shape_table infers non-core keys as measurement fields, so this table currently presents example measurements as reserved schema columns. That contradicts the sentence below and blurs the schema contract.
♻️ Proposed fix
-| `class` | object class or annotation category |
-| `confidence` | detection or classification confidence |
+Any additional columns, including `class` and `confidence`, are measurements.📝 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.
| The canonical columns are: | |
| | Column | Purpose | | |
| | ---------------- | --------------------------------------- | | |
| | `object_id` | stable object identifier | | |
| | `image_id` | source image identifier | | |
| | `label_image_id` | source label image identifier | | |
| | `label_value` | integer label value in the label raster | | |
| | `geometry` | one logical geometry value | | |
| | `centroid` | coordinate vector for object center | | |
| | `bbox` | min/max coordinate bounds | | |
| | `class` | object class or annotation category | | |
| | `confidence` | detection or classification confidence | | |
| Any additional columns are measurements. | |
| The canonical columns are: | |
| | Column | Purpose | | |
| | ---------------- | --------------------------------------- | | |
| | `object_id` | stable object identifier | | |
| | `image_id` | source image identifier | | |
| | `label_image_id` | source label image identifier | | |
| | `label_value` | integer label value in the label raster | | |
| | `geometry` | one logical geometry value | | |
| | `centroid` | coordinate vector for object center | | |
| | `bbox` | min/max coordinate bounds | | |
| Any additional columns, including `class` and `confidence`, are measurements. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/shapes.md` around lines 87 - 101, The canonical column table in
shapes.md incorrectly lists `class` and `confidence` as reserved schema columns,
which conflicts with `make_shape_table` treating them as non-core measurements.
Update the canonical-columns list near the shape schema description to keep only
the true core fields (`object_id`, `image_id`, `label_image_id`, `label_value`,
`geometry`, `centroid`, `bbox`) and move `class` and `confidence` into the
measurements/examples discussion so the schema contract stays aligned with
`make_shape_table`.
1 participant
Description
This PR adds shapes and labels capabilities alongside the existing OME-based image capabilities of OME-Arrow. These features are typically used for segmentations of objects to label - distinguishing what pixels are a part of an object from the image, or define shapes - the geometry of particular objects.
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.
Summary by CodeRabbit
New Features
Documentation
Tests