Revert "In Data Loading, Clip to Layer BoundingBox (#8551)" #8572
Conversation
This reverts commit 7275b8b.
|
Warning Rate limit exceeded@MichaelBuessemeyer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change set removes several geometry-related methods and properties across multiple files. In the geometry utility package, methods for bounding box containment, movement, and vector negation via unary operator are deleted or replaced with explicit alternatives. In the data model and service layers, methods for converting cuboids to bounding boxes and for extracting magnification vectors are removed. The binary data service's logic for clipping data to a layer bounding box is eliminated, and adjustments are made to how voxel data is indexed during cutout operations. Changes
Possibly related PRs
Suggested reviewers
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 (
|
![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 (5)
util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala (1)
9-9: Consider revertingbottomRightback tolazy val
bottomRightis now evaluated eagerly for everyBoundingBoxinstantiation, even when it is never accessed.
For tight-loop geometry code (e.g. iterating over millions of cuboids) this can introduce measurable overhead.- val bottomRight: Vec3Int = topLeft.move(width, height, depth) + lazy val bottomRight: Vec3Int = topLeft.move(width, height, depth)If you intentionally switched to strict evaluation for correctness (e.g. mutation trouble) please document the reason in a comment.
Otherwise alazy valkeeps the previous allocation behaviour without side-effects.util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (1)
56-56: Preserve the convenient unary negation operator
negateworks, but removingunary_-breaks client code that relied on-vec.
You can keep both with a one-liner alias to maintain backwards compatibility and developer ergonomics:def negate: Vec3Int = Vec3Int(-x, -y, -z) + + /** Backwards-compat convenience – allows `-vec` in addition to `vec.negate` */ + def unary_- : Vec3Int = negateNo runtime cost, and existing call-sites compile unchanged.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (1)
3-3: Unused import check
Vec3Intis imported but only referenced indirectly throughtopLeft.magandmagreturn type – both are valid.
If later refactors remove themagmethod, remember to drop this import to avoid warnings.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (2)
171-172:subsamplingStridesis hard-coded to1-1-1Right now this adds indirection and three integer divisions per inner loop iteration without changing the result.
If true subsampling support is planned later, consider guarding the optimisation behind an
if (subsamplingStrides != Vec3Int.ones)block, or calculate the division only when strides ≠ 1 to avoid the extra cost.- val subsamplingStrides = Vec3Int.ones + // Placeholder for future subsampling support – currently no-op (all ones) + val subsamplingStrides = Vec3Int.ones
197-199: Redundant division by1– micro-optimise or documentThe divisions by
subsamplingStrides.{x,y,z}are effectively… / 1. If perf-critical, inline:- val rx = (xMin - cuboid.topLeft.voxelXInMag) / subsamplingStrides.x - val ry = (y - cuboid.topLeft.voxelYInMag) / subsamplingStrides.y - val rz = (z - cuboid.topLeft.voxelZInMag) / subsamplingStrides.z + val rx = xMin - cuboid.topLeft.voxelXInMag + val ry = y - cuboid.topLeft.voxelYInMag + val rz = z - cuboid.topLeft.voxelZInMagIf you expect non-unit strides soon, add a comment so future maintainers understand the current overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
util/src/main/scala/com/scalableminds/util/geometry/BoundingBox.scala(1 hunks)util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala(0 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala(3 hunks)
💤 Files with no reviewable changes (1)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/DataServiceRequests.scala
🧰 Additional context used
🧬 Code Graph Analysis (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/requests/Cuboid.scala (1)
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (2)
Vec3Int(7-88)Vec3Int(90-156)
util/src/main/scala/com/scalableminds/util/geometry/Vec3Int.scala (1)
frontend/javascripts/libs/mjs.ts (1)
negate(384-386)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (1)
112-124: Removal of layer-bounding-box clipping – verify downstream assumptions
convertAccordingToRequestno longer zeros voxels outside the layer bounding box.
While earlier validation skips buckets outside the range, individual voxels that lie outside the layer bounding box but inside a partially overlapping bucket will now be returned untouched.If any consumers (rendering, quantification, analytics) relied on the previous zero-fill behaviour, this could re-introduce the very bug that PR #8551 tried to solve.
Please double-check:
cutOutCuboidalways keeps the cuboid fully inside the layer bounding box, or- downstream code can cope with out-of-range voxel values.
A quick run of the existing integration test‐suite that originally motivated #8551 would provide confidence.
This reverts #8551 (commit 7275b8b) due to an introduced bug. See report https://scm.slack.com/archives/C02H5T8Q08P/p1745845348102239
(Please delete unneeded items, merge only when none are left open)