Support OME-NGFF zarr dataset translation #8311
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the OME NGFF (Next Generation File Format) coordinate transformation handling in the WebKnossos datastore. The changes primarily focus on adding support for translation parameters in coordinate transformations across multiple Scala files. The modifications enable more flexible spatial representation of datasets by allowing both scaling and translation transformations, which is crucial for accurately positioning and scaling multidimensional scientific imaging data. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
Line range hint
67-67: Document potential breaking changes.Consider adding an entry to the "Breaking Changes" section if the coordinate transformation changes might affect existing datasets or integrations.
Would the changes to coordinate transformations affect existing datasets? If so, please document:
- Impact on existing datasets
- Required user actions
- Migration steps if any
This reverts commit 9a8871c.
...sos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr/NgffMetadata.scala
Outdated
Show resolved
Hide resolved
...ssos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffExplorationUtils.scala
Outdated
Show resolved
Hide resolved
| }) | ||
| layers: List[(DataLayerWithMagLocators, VoxelSize)] = layerLists.flatten | ||
| } yield layers ++ labelLayers | ||
| } yield layers //++ labelLayers |
There was a problem hiding this comment.
just a reminder to re-add this before merging
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffExplorationUtils.scala (2)
269-283: Consider simplifying the nested transformation logic.The transformation folding logic is complex and could be simplified for better maintainability. Consider extracting the transformation handling into separate functions for each type.
- var translation = firstDataset.coordinateTransformations.foldLeft(baseTranslation)((acc, ct) => { - ct.`type` match { - case "translation" => - ct.translation match { - case Some(translationList) => - acc.zipWithIndex.map { case (v, i) => v * translationList(translationList.length - 1 - i) } - case _ => acc - } - case _ => - ct.scale match { - case Some(scaleList) => acc.zipWithIndex.map { case (v, i) => v / scaleList(scaleList.length - 1 - i) } - case _ => acc - } - } - }) + def applyTranslation(acc: List[Double], ct: NgffCoordinateTransformation): List[Double] = + ct.translation.map(tl => + acc.zipWithIndex.map { case (v, i) => v * tl(tl.length - 1 - i) } + ).getOrElse(acc) + + def applyScale(acc: List[Double], ct: NgffCoordinateTransformation): List[Double] = + ct.scale.map(sl => + acc.zipWithIndex.map { case (v, i) => v / sl(sl.length - 1 - i) } + ).getOrElse(acc) + + val translation = firstDataset.coordinateTransformations.foldLeft(baseTranslation) { (acc, ct) => + if (ct.`type` == "translation") applyTranslation(acc, ct) + else applyScale(acc, ct) + }
293-296: Consider using named constants for the affine transformation matrix.The matrix construction uses magic numbers. Consider extracting these into named constants for better readability and maintainability.
+ val IDENTITY_MATRIX = List( + List(1, 0, 0), + List(0, 1, 0), + List(0, 0, 1) + ) + val coordinateTransformation = CoordinateTransformation( `type` = CoordinateTransformationType.affine, matrix = Some( - List(List(1, 0, 0, xTranslation), - List(0, 1, 0, yTranslation), - List(0, 0, 1, zTranslation), - List(0, 0, 0, 1))) + IDENTITY_MATRIX.zipWithIndex.map { case (row, i) => + row :+ (if (i < 3) translation(i) else 0) + } :+ List(0, 0, 0, 1))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr/NgffMetadata.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr/SharedNgffMetadataAttributes.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffExplorationUtils.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr/SharedNgffMetadataAttributes.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr/NgffMetadata.scala
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffExplorationUtils.scala (2)
24-25: LGTM!The new imports are properly organized and necessary for the coordinate transformation functionality.
264-302: Add unit tests for the translation handling.The new translation handling logic is complex and handles multiple cases (2D/3D, different transformation types). Consider adding comprehensive unit tests to verify the behavior.
Would you like me to help generate unit test cases covering various scenarios including:
- 2D and 3D datasets
- Different transformation combinations
- Edge cases (empty translations, invalid lengths)
| ct.translation match { | ||
| case Some(translationList) => | ||
| acc.zipWithIndex.map { case (v, i) => v * translationList(translationList.length - 1 - i) } | ||
| case _ => acc |
There was a problem hiding this comment.
💡 Codebase verification
The review comment is valid and the suggested fix is necessary. The code assumes the translation list has sufficient elements without validation, which could lead to IndexOutOfBoundsException. The same unsafe pattern is also present in scale list handling. The suggested fix appropriately handles cases where the translation list is shorter than expected.
🔗 Analysis chain
Add validation for translation list length.
The code assumes the translation list has sufficient elements but doesn't validate this assumption. Consider adding length validation to prevent potential index out of bounds exceptions.
case "translation" =>
ct.translation match {
case Some(translationList) =>
+ if (translationList.length < acc.length) acc
+ else
acc.zipWithIndex.map { case (v, i) => v * translationList(translationList.length - 1 - i) }
case _ => acc
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Get the full context of the file
cat webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffExplorationUtils.scala
# Look for test files related to NgffExplorationUtils
fd "NgffExplorationUtils.*Test.*scala"
# Search for other usages of translation in the codebase
rg "translation" --type scala -A 2 -B 2
Length of output: 25250
Script:
#!/bin/bash
# Look for test files that might contain translation-related tests
rg -B 3 -A 3 "translation.*Test" --type scala
# Look for any GitHub issues mentioning translation and index
gh issue list -s all -S "translation index bounds"
# Search for potential error logs or exception handling related to translation
rg -B 2 -A 2 "IndexOutOfBoundsException.*translation" --type scala
Length of output: 169
Script:
#!/bin/bash
# Look for NgffCoordinateTransformation construction
rg "new NgffCoordinateTransformation|NgffCoordinateTransformation\(" --type scala -B 2 -A 2
# Look for similar array access patterns with length checks
rg "length.*\- 1.*\-.*i" --type scala -B 2 -A 2
# Look for validation patterns in NGFF code
rg "validate|check" --type scala --glob "*Ngff*.scala" -B 2 -A 2
Length of output: 4063
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffV0_4Explorer.scala (1)
71-71: LGTM! Consider adding documentation.The coordinate transformations are correctly applied to both layer types. Consider adding documentation to describe:
- The expected format of coordinate transformations
- How translations affect the layer positioning
- Any limitations or constraints on transformation values
Also applies to: 83-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffV0_4Explorer.scala(3 hunks)
🔇 Additional comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NgffV0_4Explorer.scala (2)
61-61: LGTM! Verify translation extraction for edge cases.The addition of translation support aligns with the PR objectives. Consider verifying the behavior when:
- Translation data is missing
- Translation data is malformed
- Translation is present but contains invalid values
Line range hint
39-42: Critical: Re-enable label layers before merging.As mentioned in the PR objectives and past review comments, label layers need to be re-enabled before merging. Currently, the label layers functionality appears to be commented out.
Run this script to verify the commented-out label layer code:
2 participants
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)