Remove VoxelType Enum, use ArrayDataType instead for WKW #8559
Conversation
📝 WalkthroughWalkthroughThis change removes the Changes
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/WKWBucketStreamSink.scala (1)
21-23: Confirm parameter order for theWKWHeaderfactory
WKWHeader’s companionapplyexpects
(numChunksPerShardDimension, numVoxelsPerChunkDimension, blockType, dataType, numChannels).
Here we passWKWHeader(1, DataLayer.bucketLength, ChunkType.LZ4, dataType, numChannels)This maps
1 → numChunksPerShardDimensionandDataLayer.bucketLength → numVoxelsPerChunkDimension, i.e. we create shards that contain exactly one chunk whose side–length equalsbucketLength.
That is exactly what the old implementation did, so it is probably intended – but please double-check that:
DataLayer.bucketLengthstill equals the side length (in voxels) of one chunk, not of the whole shard.- All readers/writers that consume these WKW files rely on
numChunksPerShardDimension == 1.
If we ever decide to pack several chunks per file, this hard-coded1will become a silent bug.If the assumption is correct, consider replacing the magic constant with a named value for self-documentation, e.g.
- val header = WKWHeader(1, DataLayer.bucketLength, ChunkType.LZ4, dataType, numChannels) + val NumChunksPerShard = 1 + val header = WKWHeader(NumChunksPerShard, DataLayer.bucketLength, ChunkType.LZ4, dataType, numChannels)webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/WKWHeader.scala (3)
34-39: Channel count now explicit – great, but rename parameters in docsSwitching from the implicit
numBytesPerVoxelto the explicit(dataType, numChannels)clears up previous ambiguity – nice!
Minor nit: the scaladoc / error messages inside the file still refer to voxelType in a few places (e.g. line 133).
Renaming those to dataType will prevent confusion for future maintainers.
150-152: Variable name still references removed concept
voxelTypeIdandvoxelTypevariables are now holding an ArrayDataType.
Renaming todataTypeId/dataTypewould align the implementation with the refactor and avoid cognitive friction.
175-184: Factoryapplyoverload: consider defaultingnumChannelsto 1Most call-sites will create single-channel headers. Providing
def apply(numChunksPerShardDimension: Int, numVoxelsPerChunkDimension: Int, blockType: ChunkType.Value, dataType: ArrayDataType, numChannels: Int = 1): WKWHeader = …reduces boilerplate and makes the common case explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/VoxelType.scala(0 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/WKWHeader.scala(8 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/ArrayDataType.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/BytesConverter.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/DatasetHeader.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/WKWBucketStreamSink.scala(2 hunks)
💤 Files with no reviewable changes (1)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/VoxelType.scala
🧰 Additional context used
🧬 Code Graph Analysis (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/BytesConverter.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/ArrayDataType.scala (2)
ArrayDataType(5-78)bytesPerElement(9-23)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/WKWBucketStreamSink.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/WKWHeader.scala (3)
ChunkType(25-29)WKWHeader(31-113)WKWHeader(115-186)webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/wkw/WKWFile.scala (1)
WKWFile(68-115)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (3)
DataLayer(283-329)ElementClass(37-195)toArrayDataTypeAndChannel(182-194)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (2)
frontend/javascripts/types/api_flow_types.ts (1)
ElementClass(33-44)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/ArrayDataType.scala (1)
ArrayDataType(5-78)
🔇 Additional comments (8)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
182-194: Looks good: Implementation of toArrayDataTypeAndChannel is well-structured and correctThe new method properly maps each
ElementClassvalue to appropriateArrayDataTypeand channel count pairs, with special handling foruint24which uses 3 channels. This implementation supports the PR's goal of replacingVoxelTypewith directArrayDataTypeusage.webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/BytesConverter.scala (2)
3-3: Import statement updated correctlyThe import has been streamlined to only include the necessary
ArrayDataTypetype definition.
13-13: Method call updated to use ArrayDataType.bytesPerElementChanged from the removed
bytesPerElementForto align with the refactoring inArrayDataType.scala.webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/DatasetHeader.scala (2)
6-6: Import simplified correctlyImport statement updated to only include
ArrayDataType.ArrayDataType, removing unnecessary imports.
31-31: Method call updated to ArrayDataType.bytesPerElementCorrectly changed to use the renamed method in
ArrayDataTypeobject.webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/ArrayDataType.scala (3)
9-9: Method renamed from bytesPerElementFor to bytesPerElementThe method name has been updated for better consistency while preserving the same functionality.
51-63: Well-implemented toWKWId mapping methodThis method provides a clean mapping from ArrayDataType values to WKW type ID integers, supporting the refactoring away from VoxelType. The pattern matching is exhaustive and covers all data types.
65-77: Well-implemented fromWKWTypeId conversion methodThis method correctly maps WKW type IDs back to ArrayDataType values, providing the reverse functionality of toWKWId. The pattern matching is exhaustive and handles all possible WKW type IDs.
| private def numBytesPerVoxel = numChannels * ArrayDataType.bytesPerElement(dataType) | ||
|
|
There was a problem hiding this comment.
Potential overflow & spec violation for numBytesPerVoxel
numBytesPerVoxel is written with output.writeByte, i.e. only one unsigned byte is stored in the header.
With the new formula
private def numBytesPerVoxel = numChannels * ArrayDataType.bytesPerElement(dataType)values above 255 will silently overflow or truncate, corrupting every subsequent reader.
Typical cases (u1×3, f4×1, …) are safe, but nothing prevents the caller from creating, say, an f8 with 4 channels (≈ 32 bytes).
+ require(
+ numBytesPerVoxel <= 0xFF,
+ s"numBytesPerVoxel=$numBytesPerVoxel exceeds 1-byte limit of WKW header"
+ )Add the guard (ideally right after the calculation) and document the limit in the companion apply that builds headers.
| output.writeByte(blockType.id) | ||
| output.writeByte(voxelType.id) | ||
| output.writeByte(ArrayDataType.toWKWId(dataType)) | ||
| output.writeByte(numBytesPerVoxel) | ||
| if (isHeaderFile) { |
There was a problem hiding this comment.
Missing validation for numChannels when reading header
During deserialization we do:
val numChannels = numBytesPerVoxel / ArrayDataType.bytesPerElement(voxelType)If numBytesPerVoxel is not an exact multiple, integer division truncates and we silently drop fractional channels.
Please validate divisibility:
+ val bytesPerElem = ArrayDataType.bytesPerElement(voxelType)
+ require(numBytesPerVoxel % bytesPerElem == 0,
+ s"Inconsistent header: $numBytesPerVoxel not divisible by $bytesPerElem")
+ val numChannels = numBytesPerVoxel / bytesPerElem
Removes the VoxelType Enum, and adapts the WKW code to use ArrayDataType directly.
I decided to leave Zarr3DataType, N5DataType and PrecomputedDataType untouched, as their json formats are useful to parse the respective headers easily.
Steps to test:
TODOs:
Issues: