Remove explicit returns #8205
Remove explicit returns #8205
Conversation
WalkthroughThe pull request introduces several modifications across four files. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@frcroth has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 24 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (4)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataFinder.scala (2)
9-17: LGTM! Consider adding documentation for the voxel offset calculation.The Iterator.tabulate implementation is clean and functional. However, the voxel offset calculation logic could benefit from a brief comment explaining the dimensional scaling.
private def getExactDataOffset(data: Array[Byte], bytesPerElement: Int): Vec3Int = { val bucketLength = DataLayer.bucketLength + // Calculate voxel offsets in 3D space: + // x: scaled by bytesPerElement + // y: scaled by bytesPerElement * bucketLength (row stride) + // z: scaled by bytesPerElement * bucketLength^2 (slice stride) Iterator .tabulate(bucketLength, bucketLength, bucketLength) { (z, y, x) =>
18-24: Consider simplifying the nested flatten calls.The current implementation works well but could be more concise by using flatten.flatten.
} - .flatten - .flatten + .flatten.flatten .collectFirst {Alternatively, you could use Iterator.tabulate's single-dimensional form with index division/modulo for a potentially more efficient approach:
Iterator .tabulate(bucketLength * bucketLength * bucketLength) { i => val x = i % bucketLength val y = (i / bucketLength) % bucketLength val z = i / (bucketLength * bucketLength) val scaledX = x * bytesPerElement val scaledY = y * bytesPerElement * bucketLength val scaledZ = z * bytesPerElement * bucketLength * bucketLength (x, y, z, scaledX + scaledY + scaledZ) } .collectFirst { case (x, y, z, voxelOffset) if data.slice(voxelOffset, voxelOffset + bytesPerElement).exists(_ != 0) => Vec3Int(x, y, z) } .getOrElse(Vec3Int.zeros)webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/CumsumParser.scala (1)
54-60: Consider extracting timing logic for better separation of concerns.While the timing measurement is now correctly placed after parsing completion, consider extracting it to a separate wrapper method for better separation of concerns.
Here's a suggested refactoring:
def parse(f: File, maxReaderRange: Long): BoundingBoxCache = - parseImpl(f, maxReaderRange, List(), System.currentTimeMillis()) + withTiming("Cumsum parsing") { + parseImpl(f, maxReaderRange, List()) + } + private def withTiming[T](operation: String)(block: => T): T = { + val start = System.currentTimeMillis() + val result = block + val end = System.currentTimeMillis() + logger.info(s"$operation took ${end - start} ms") + result + } def parseImpl(f: File, maxReaderRange: Long, - initialBoundingBoxList: List[(Long, Long, Long, Long, Long, Long)], - start: Long): BoundingBoxCache = { + initialBoundingBoxList: List[(Long, Long, Long, Long, Long, Long)]): BoundingBoxCache = { // ... existing implementation ... - val end = System.currentTimeMillis() - logger.info(s"Cumsum parsing took ${end - start} ms") new BoundingBoxCache(cache, BoundingBoxFinder(positionSets._1, positionSets._2, positionSets._3, minBoundingBox), maxReaderRange)This change:
- Separates timing concerns from parsing logic
- Makes timing reusable across other operations
- Maintains functional style without explicit returns
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AdHocMeshService.scala (1)
136-148: Consider improving readability with better formatting and documentationWhile the implementation is correct, consider these readability improvements:
def subVolumeContainsSegmentId[T]( data: Array[T], dataDimensions: Vec3Int, boundingBox: BoundingBox, segmentId: T ): Boolean = { + // Check if the segment exists within the bounding box by calculating + // the linear voxel offset for each position in 3D space boundingBox.topLeft.x until boundingBox.bottomRight.x exists { x => boundingBox.topLeft.y until boundingBox.bottomRight.y exists { y => boundingBox.topLeft.z until boundingBox.bottomRight.z exists { z => + // Convert 3D coordinates to linear array index val voxelOffset = x + y * dataDimensions.x + z * dataDimensions.x * dataDimensions.y data(voxelOffset) == segmentId } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AdHocMeshService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataFinder.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/CumsumParser.scala(1 hunks)
🔇 Additional comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/CumsumParser.scala (1)
51-52: LGTM! Successfully eliminated explicit return.
The changes effectively remove the explicit return statement while maintaining the correct control flow. This aligns well with Scala's best practices and fixes the NonLocalReturn concerns mentioned in issue #6356.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AdHocMeshService.scala (1)
136-148: Successfully removed explicit returns using idiomatic Scala!
The refactoring of subVolumeContainsSegmentId effectively eliminates explicit returns by using exists, which aligns perfectly with the PR's objective. The implementation maintains the same functionality while being more idiomatic Scala.
There was a problem hiding this comment.
Very nice! Cumsum parsing and AdHoc meshing LGTM and appear to work :) Unfortunately, the findData route now always fails for me. I tested with the color layers of several datasets including l4_sample. Did it work for you? If so, maybe we can debug this together on my laptop 🤷
The CI rule is not that important IMO. Then let’s take care of this when doing reviews :)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (1)
Line range hint
1054-1076: Bug: Incorrect variable reference in condition.There's a potential bug in the error handling condition:
if ((!position || !resolution) && volume.fallbackLayer) {The variable
resolutionis undefined as it was renamed tomag. This condition should be updated.Apply this fix:
-if ((!position || !resolution) && volume.fallbackLayer) { +if ((!position || !mag) && volume.fallbackLayer) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/admin/admin_rest_api.ts(2 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(3 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (2)
1029-1032: Variable rename from foundResolution to foundMag for better terminology consistency.
The change aligns with the PR's objective to use more accurate terminology. The term mag (magnification) is more precise than resolution in this context.
1043-1051: Verify the destructuring of mag property.
The code now expects a mag property from the API responses of both findDataPositionForVolumeTracing and findDataPositionForLayer. Ensure that these API endpoints have been updated to return the mag property instead of resolution.
frontend/javascripts/admin/admin_rest_api.ts (2)
1549-1558: Verify API consumers are updated for the resolution → mag change.
The change from resolution to mag in the return type and implementation looks good. Please ensure all consumers of this API are updated to handle the new property name.
1567-1574: LGTM! Consistent with findDataPositionForLayer changes.
The change from resolution to mag is consistently applied here, maintaining parallel structure with the findDataPositionForLayer function.
URL of deployed dev instance (used for testing):
I did not find a linting option in scalac to find return statements (-Xlint:nonlocal-return seems to be something different). https://docs.scala-lang.org/overviews/compiler-options/index.html
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor