Add support to explore compact-style n5 multiscales metadata #8456
Conversation
📝 WalkthroughWalkthroughThis pull request adds support for N5 datasets using compact-style multiscale metadata. It introduces a new metadata case class with JSON formatting and extends exploration functionality across local, remote, and Neuroglancer interfaces by adding new explorer classes and methods. The changes include utility method additions and inheritance refactoring to centralize N5 data exploration without impacting existing features. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 0
🧹 Nitpick comments (6)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)
94-110: Consider adding a comment about explorer orderingThe ordering of explorers is important as it determines priority. While there's a basic comment on line 98, consider adding a more detailed comment explaining why
N5CompactMultiscalesExploreris positioned where it is in the hierarchy.// Explorers are ordered to prioritize the explorer reading meta information over raw Zarr, N5, ... data. +// N5 explorers are ordered from more specific (N5MultiscalesExplorer, N5CompactMultiscalesExplorer) to more general (N5ArrayExplorer)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala (1)
91-97: Add expanded diagnostics for incomplete metadata.
The newexploreLocalN5CompactMultiscalesmethod parallels existing local explorers, boosting consistency. Consider more detailed logging to help diagnose partial or malformed compact-style metadata.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5MultiscalesExplorer.scala (2)
34-34: Refactor to uselayerFromMagsWithAttributes.
Delegating layer construction tolayerFromMagsWithAttributesremoves duplication. Double-check that any prior bounding-box or element-class validations still occur as intended.
71-71: Relax or clarify strict power-of-two requirement.
Requiring power-of-two mags may exclude datasets that lack uniform downsampling. Consider either a more descriptive error or a fallback path for non-power-of-two magnifications.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5Explorer.scala (1)
28-44: Consider supporting additional length units.
While this robust mapping covers many SI prefixes, you might consider including the angstrom symbol “Å” (with factor 0.1 nm) if there's a possibility of encountering that unit.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5CompactMultiscalesExplorer.scala (1)
20-37: Refine error messaging for multiScale requirement.
WhenmultiScaleis missing or false, the error text “does have multiScale=true” might be confusing for users. Consider rephrasing for clarity, e.g. “Expected multiScale=true but found otherwise.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.unreleased.md(1 hunks)util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/n5/N5Metadata.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala(2 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5ArrayExplorer.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5CompactMultiscalesExplorer.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5Explorer.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5MultiscalesExplorer.scala(3 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NeuroglancerUriExplorer.scala(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (23)
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (1)
82-87: Nice implementation ofisAllPowersOfTwomethod!The method correctly checks if all components of a Vec3Int are powers of two using an efficient bit manipulation technique. This is an elegant solution that will be useful for validating properties of N5 compact multiscales metadata.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5ArrayExplorer.scala (1)
16-16: Inheritance change looks goodChanging the parent class from
RemoteLayerExplorertoN5Explorerfollows good object-oriented design by creating a more specific hierarchy for N5-related explorers.CHANGELOG.unreleased.md (1)
16-16: LGTM - Well-documented changelog entryThe changelog entry properly documents the new feature of supporting N5 datasets with compact-style multiscale metadata, with a reference to the PR number.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)
105-105: Integration of new explorer looks goodThe
N5CompactMultiscalesExplorerhas been appropriately added to the list of explorers, positioned betweenN5MultiscalesExplorerandN5ArrayExplorer. This ordering follows a logical progression from more specialized to more general explorers.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NeuroglancerUriExplorer.scala (1)
57-61: Add a fallback check for unsuccessful explorers.
Extending the sequence withN5CompactMultiscalesExplorerenhances coverage of N5 dataset variants. Ensure that each explorer gracefully defers to the next if it fails or encounters incompatible metadata.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala (1)
39-39: Confirm error handling among the local explorers sequence.
InsertingexploreLocalN5CompactMultiscalesalongside other explorers helps maintain uniform exploration logic. Verify that it properly short-circuits or combines results when certain metadata are incomplete.webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/n5/N5Metadata.scala (1)
23-36: Revisit default resolution logic.
While the PR objectives mention falling back to a[1,1,1]resolution if not provided, the code requiresresolutionas a non-optional field. Confirm whether an explicit fallback or an exception is desired here.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5MultiscalesExplorer.scala (3)
7-7: Confirm necessity of this import.
ImportingN5Layeris consistent with returning the specialized layer type from this explorer. There are no apparent conflicts.
14-14: Favor composition viaN5Explorer.
Switching toextends N5Explorercentralizes common logic, which should increase clarity and facilitate code reuse.
67-67: Scrutinize dimension validation inmagFromTransform.
Ensure that the transform’s axes and scale values are fully validated before deriving magnification. Incomplete or mismatched axis data could lead to incorrect magnification vectors.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5Explorer.scala (7)
1-2: No critical concerns.
3-10: No issues with import statements.
13-16: Trait declaration looks good.
The usage ofExecutionContextis cleanly passed as an implicit, aligning well with async/await patterns.
17-26: Validate length of units list.
Currently, the code assumes that theunitssequence has entries for x, y, z (and possibly c). Consider verifying that the list is at least as long as the largest axis index to avoid anIndexOutOfBoundsException.
46-57: Logic for extracting axis order is correct.
This ensures x, y, z (and optionally c) are properly matched. No further issues noted.
58-60: Voxel size extraction is straightforward.
The fallback to a failure on out-of-range indexing is acceptable given preceding checks. Looks good.
61-72: Layer creation approach is coherent.
Dynamically picking a segmentation or data layer based on naming and element class is a clear design. Code is well-organized.webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/N5CompactMultiscalesExplorer.scala (6)
1-2: No critical concerns.
3-13: Import statements look good.
16-16: Class definition follows expected pattern.
ExtendingN5Explorerand mixing inFoxImplicitsis a logical choice to reuse shared methods and Fox utilities.
18-18: Naming is clear.
Thenamemethod concisely denotes the purpose of this explorer.
38-45: Consistent voxel size extraction.
The multiplication of axis unit factors with resolution aligns with the logic inN5Explorer. No issues found.
46-67: Ensure downsampling factors match the axis count.
IfdownsamplingFactordoesn’t provide enough indices for x, y, or z, a runtime exception could occur. Confirm that minimal length requirements are satisfied or gracefully handle short lists.
| metadataPath <- Fox.successful(remotePath / N5Metadata.FILENAME_ATTRIBUTES_JSON) | ||
| n5Metadata <- metadataPath | ||
| .parseAsJson[N5CompactMultiscalesMetadata] ?~> s"Failed to read N5 header at $metadataPath" | ||
| _ <- bool2Fox(n5Metadata.multiScale.contains(true)) ?~> s"N5 header at $metadataPath does have multiScale=true" |
There was a problem hiding this comment.
What does this line do? It seems like it checks that multiScale has true and fails otherwise, but the error says that the problem is that it has true.
There was a problem hiding this comment.
It’s a typo in the error message. I’ll add a not!
2 participants
A new format for N5 multiscales metadata has cropped up, it is described in this neuroglancer doc. It has a totally different structure from the multiscales description used for example in this openorganelle dataset.
Example new attributes.json content:
This PR adds support for exploring such datasets.
Some caveats: the
downsamplingFactors/scalesmust be described in the toplevel;resolution/unitsmust also be described in the toplevel, otherwise we fall back to 1,1,1nm. Let’s add support for the other cases once we see them.Steps to test:
n5://gs://ucl-hip-ct-35a68e99feaae8932b1d44da0358940b/LADAF-2021-17/heart/19.85um_complete-organ_bm18/– should show mags from 1-1-1 to 16-16-16 and load data; show the correct voxel sizeIssues: