Relax bounding box requirements for model training #8222
Conversation
…as well as the same dimensions overall for model training
…ax-train-model-bbox-requirements
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
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
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
68-68: LGTM with a minor suggestion for clarityThe warning message accurately reflects the relaxed requirements. Consider adding an example for better clarity:
- message="Please note that this feature is experimental. All bounding boxes should have equal dimensions or have dimensions which are multiples of the smallest bounding box. Ensure the size is not too small (we recommend at least 10 Vx per dimension) and choose boxes that represent the data well." + message="Please note that this feature is experimental. All bounding boxes should have equal dimensions or have dimensions which are multiples of the smallest bounding box (e.g., if smallest box is 10x10, others can be 10x10, 20x20, or 10x20). Ensure the size is not too small (we recommend at least 10 Vx per dimension) and choose boxes that represent the data well."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(2 hunks)
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
426-426: Verify impact on model training quality
The relaxed validation allows for more flexible bounding box configurations, but we should verify:
- That the training system can handle variable box sizes
- That model quality isn't negatively impacted
- That the test steps in the PR description cover all edge cases
| areSomeBBoxesInvalid: true, | ||
| invalidBBoxesReason: "All bounding boxes must have the same size.", | ||
| }; | ||
| return { areSomeBBoxesInvalid: false, invalidBBoxesReason: null }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding basic size validations
While removing strict size equality checks aligns with the PR objective, consider adding basic validations to prevent training issues:
- Validate minimum box size (10 Vx per dimension)
- Validate that box dimensions are multiples of the smallest box
- Add debug logging of box dimensions for troubleshooting
if (userBoundingBoxes.length === 0) {
return {
areSomeBBoxesInvalid: true,
invalidBBoxesReason: "At least one bounding box must be defined.",
};
}
+ // Find smallest dimensions
+ const minDimensions = userBoundingBoxes.reduce(
+ (min, box) => ({
+ x: Math.min(min.x, box.boundingBox.width),
+ y: Math.min(min.y, box.boundingBox.height),
+ z: Math.min(min.z, box.boundingBox.depth),
+ }),
+ { x: Infinity, y: Infinity, z: Infinity }
+ );
+
+ // Validate minimum size and multiple requirements
+ const invalidBoxes = userBoundingBoxes.filter(box => {
+ const dims = box.boundingBox;
+ const tooSmall = dims.width < 10 || dims.height < 10 || dims.depth < 10;
+ const notMultiple =
+ dims.width % minDimensions.x !== 0 ||
+ dims.height % minDimensions.y !== 0 ||
+ dims.depth % minDimensions.z !== 0;
+ return tooSmall || notMultiple;
+ });
+
+ if (invalidBoxes.length > 0) {
+ return {
+ areSomeBBoxesInvalid: true,
+ invalidBBoxesReason: `Some bounding boxes are either too small (min 10 Vx) or not multiples of the smallest box dimensions (${minDimensions.x}x${minDimensions.y}x${minDimensions.z})`
+ };
+ }
+
+ // Log dimensions for debugging
+ console.debug('Bounding box dimensions:', userBoundingBoxes.map(box => ({
+ width: box.boundingBox.width,
+ height: box.boundingBox.height,
+ depth: box.boundingBox.depth
+ })));
+
return { areSomeBBoxesInvalid: false, invalidBBoxesReason: null };Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@daniel-wer Maybe these good suggestions:
- Validate minimum box size (10 Vx per dimension)
- Validate that box dimensions are multiples of the smallest box
Maybe showing a warning in the form (afaik antd supports this), making the from still submittable but also showing the user potential issues with the training data up front.
I'd let this for you to decide whether to add this in this pr :)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-20: LGTM with a minor suggestion for clarity.The changelog entry accurately describes the changes and includes helpful recommendations. Consider slightly rewording for better clarity:
-Allowed to train an AI model using differently sized bounding boxes. We recommend all bounding boxes to have equal dimensions or to have dimensions which are multiples of the smallest bounding box. +Added support for training AI models with differently sized bounding boxes. For optimal results, we recommend using bounding boxes with either equal dimensions or dimensions that are multiples of the smallest bounding box.
| areSomeBBoxesInvalid: true, | ||
| invalidBBoxesReason: "All bounding boxes must have the same size.", | ||
| }; | ||
| return { areSomeBBoxesInvalid: false, invalidBBoxesReason: null }; |
There was a problem hiding this comment.
@daniel-wer Maybe these good suggestions:
- Validate minimum box size (10 Vx per dimension)
- Validate that box dimensions are multiples of the smallest box
Maybe showing a warning in the form (afaik antd supports this), making the from still submittable but also showing the user potential issues with the training data up front.
I'd let this for you to decide whether to add this in this pr :)
…ax-train-model-bbox-requirements
… errors and warnings more prominent by using Alerts. Include annotation ID and topleft + size for offending boxes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
489-503: Replace map with forEach since the return value is unused.The
mapfunction is being used for its side effects only, which is not ideal.- userBoundingBoxes.map(({ boundingBox: box, annotationId }) => { + userBoundingBoxes.forEach(({ boundingBox: box, annotationId }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
69-70: LGTM! Clear and informative alert message.
The updated alert message effectively communicates the new requirements while providing helpful guidance about bounding box dimensions.
220-225: LGTM! Good enhancement to bounding box tracking.
The addition of annotation IDs to bounding boxes improves traceability and error reporting capabilities.
459-483: LGTM! Robust bounding box validation logic.
The validation logic effectively implements the new requirements while maintaining good checks for minimum dimensions.
347-374: LGTM! Well-structured error and warning display.
The UI effectively distinguishes between errors (blocking) and warnings (non-blocking), providing a good user experience while maintaining data quality.
|
@MichaelBuessemeyer Thanks for the review. I opted to refactor the error display mechanism and include warnings as well. This turned out to be a bit more involved, but I think the result is more user friendly. Please have a look at my latest commit. See this screenshot, but feel free to test yourself as well :)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
231-242: Consider enhancing error messages with actionable guidanceWhile the error/warning separation is well-implemented, consider adding specific remediation steps to help users resolve issues more effectively.
const { hasAnnotationErrors, errors: annotationErrors } = checkAnnotationsForErrorsAndWarnings(annotationInfos); const { hasBBoxErrors, hasBBoxWarnings, errors: bboxErrors, warnings: bboxWarnings, } = checkBoundingBoxesForErrorsAndWarnings(userBoundingBoxes); const hasErrors = hasAnnotationErrors || hasBBoxErrors; const hasWarnings = hasBBoxWarnings; -const errors = [...annotationErrors, ...bboxErrors]; -const warnings = bboxWarnings; +const errors = [...annotationErrors, ...bboxErrors].map(error => + `${error}\nTo fix this: Check the annotation settings and ensure all required bounding boxes are properly defined.` +); +const warnings = bboxWarnings.map(warning => + `${warning}\nRecommendation: Consider adjusting the bounding box dimensions to match the suggested criteria.` +);
377-385: Enhance tooltip message specificityConsider providing more specific guidance in the tooltip when the form is disabled.
- <Tooltip title={hasErrors ? "Solve the errors displayed above before continuing." : ""}> + <Tooltip title={hasErrors ? "Please address all validation errors above. Each error must be resolved before training can begin." : "Click to start the model training process"}>
476-483: Consider optimizing dimension calculationThe current implementation recalculates dimensions for each box. Consider memoizing the results for better performance with large datasets.
+ const getDimensions = React.useMemo(() => (box: Vector6) => ({ + x: box.max[0] - box.min[0], + y: box.max[1] - box.min[1], + z: box.max[2] - box.min[2], + }), []); + const minDimensions = userBoundingBoxes.reduce( - (min, { boundingBox: box }) => ({ - x: Math.min(min.x, box.max[0] - box.min[0]), - y: Math.min(min.y, box.max[1] - box.min[1]), - z: Math.min(min.z, box.max[2] - box.min[2]), - }), + (min, { boundingBox: box }) => { + const dims = getDimensions(box); + return { + x: Math.min(min.x, dims.x), + y: Math.min(min.y, dims.y), + z: Math.min(min.z, dims.z), + }; + }, { x: Number.POSITIVE_INFINITY, y: Number.POSITIVE_INFINITY, z: Number.POSITIVE_INFINITY }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (2)
69-70: LGTM: Improved warning message clarity
The updated message better communicates the relaxed requirements while maintaining helpful guidance about recommended dimensions.
220-225: LGTM: Enhanced bounding box tracking
Good enhancement to include annotationId with each bounding box, enabling more precise error reporting and validation.
There was a problem hiding this comment.
Nice, looks very good.
I only found potential mini improvements for the messages.
Please note that I did not test starting an AI job with multiple annotations as I don't know how and your screenshots already shows that it seems to work :)
I'd say this is ready to got; with or without the minor improvements I suggested
| const arrayBox = computeArrayFromBoundingBox(box); | ||
| const [_x, _y, _z, width, height, depth] = arrayBox; | ||
| if (width < 10 || height < 10 || depth < 10) { | ||
| tooSmallBoxes.push({ boundingBox: arrayBox, annotationId }); |
There was a problem hiding this comment.
I'd say it would be very helpful to include the bounding box name in the warning (it needs to be included in the pushed object here)
A list like
133, 115, 128, 64, 60, 25 (6745e0d1660100dd04d88932)
34, 162, 128, 82, 71, 32 (6745e0d1660100dd04d88932)
70, 79, 128, 1, 79, 46 (6745e0d1660100dd04d88932)
is not very useful. One might be able to find the bounding box listed in the waring by name much easier. I could imagine a list kinda like this:
Bounding box 1: 133, 115, 128, 64, 60, 25 (6745e0d1660100dd04d88932)
Bounding box 2: 34, 162, 128, 82, 71, 32 (6745e0d1660100dd04d88932)
Bounding box 4: 70, 79, 128, 1, 79, 46 (6745e0d1660100dd04d88932)
I am not sure whether that's the best way to include the bbox name somewhere. So feel free to do it the way you prefer.
…ax-train-model-bbox-requirements
|
Thanks for the feedback, this is what the warnings look like now:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (2)
231-242: Consider separating error/warning state managementThe error and warning state management logic could be extracted into a custom hook for better reusability and separation of concerns.
+function useErrorWarningState(annotationInfos: any[], userBoundingBoxes: any[]) { + const { hasAnnotationErrors, errors: annotationErrors } = + checkAnnotationsForErrorsAndWarnings(annotationInfos); + const { + hasBBoxErrors, + hasBBoxWarnings, + errors: bboxErrors, + warnings: bboxWarnings, + } = checkBoundingBoxesForErrorsAndWarnings(userBoundingBoxes); + + return { + hasErrors: hasAnnotationErrors || hasBBoxErrors, + hasWarnings: hasBBoxWarnings, + errors: [...annotationErrors, ...bboxErrors], + warnings: bboxWarnings, + }; +}
508-513: Consider adding dimension details to the warning messageThe warning message for small boxes could be more informative by including the minimum required dimensions.
warnings.push( - `The following bounding boxes are not at least 10 Vx in each dimension which is suboptimal for the training:\n${tooSmallBoxesStrings.join("\n")}`, + `The following bounding boxes are smaller than the minimum required dimensions (10 x 10 x 10 Vx) which is suboptimal for the training:\n${tooSmallBoxesStrings.join("\n")}`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
69-70: LGTM: Clear and informative warning message
The updated warning message effectively communicates the requirements and recommendations for bounding box dimensions.
220-225: LGTM: Improved type safety with annotationId
The mapping of user bounding boxes now includes the annotationId, which enhances type safety and traceability.
347-374: LGTM: Well-structured error and warning display
The error and warning alerts are well-organized with appropriate styling and icons. The use of whiteSpace: "pre-line" ensures proper formatting of multi-line messages.
505-507: LGTM: Clear and informative box formatting
The boxWithIdToString helper function provides a clear and consistent format for displaying bounding box information.
| userBoundingBoxes.forEach(({ boundingBox: box, name, annotationId }) => { | ||
| const arrayBox = computeArrayFromBoundingBox(box); | ||
| const [_x, _y, _z, width, height, depth] = arrayBox; | ||
| if (width < 10 || height < 10 || depth < 10) { | ||
| tooSmallBoxes.push({ boundingBox: arrayBox, name, annotationId }); | ||
| } | ||
|
|
||
| if ( | ||
| width % minDimensions.x !== 0 || | ||
| height % minDimensions.y !== 0 || | ||
| depth % minDimensions.z !== 0 | ||
| ) { | ||
| nonMultipleBoxes.push({ boundingBox: arrayBox, name, annotationId }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider early return for invalid dimensions
The validation loop should return early if any box has invalid dimensions (zero or negative) to prevent unnecessary processing.
userBoundingBoxes.forEach(({ boundingBox: box, name, annotationId }) => {
const arrayBox = computeArrayFromBoundingBox(box);
const [_x, _y, _z, width, height, depth] = arrayBox;
+ // Check for invalid dimensions
+ if (width <= 0 || height <= 0 || depth <= 0) {
+ tooSmallBoxes.push({ boundingBox: arrayBox, name, annotationId });
+ return;
+ }
+
if (width < 10 || height < 10 || depth < 10) {
tooSmallBoxes.push({ boundingBox: arrayBox, name, annotationId });
}Committable suggestion skipped: line range outside the PR's diff.
| const minDimensions = userBoundingBoxes.reduce( | ||
| (min, { boundingBox: box }) => ({ | ||
| x: Math.min(min.x, box.max[0] - box.min[0]), | ||
| y: Math.min(min.y, box.max[1] - box.min[1]), | ||
| z: Math.min(min.z, box.max[2] - box.min[2]), | ||
| }), | ||
| { x: Number.POSITIVE_INFINITY, y: Number.POSITIVE_INFINITY, z: Number.POSITIVE_INFINITY }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for zero or negative dimensions
While the code handles minimum size checks, it should also validate for zero or negative dimensions to prevent potential issues.
const minDimensions = userBoundingBoxes.reduce(
(min, { boundingBox: box }) => ({
+ // Validate that dimensions are positive
+ x: box.max[0] <= box.min[0] ? Infinity : Math.min(min.x, box.max[0] - box.min[0]),
+ y: box.max[1] <= box.min[1] ? Infinity : Math.min(min.y, box.max[1] - box.min[1]),
+ z: box.max[2] <= box.min[2] ? Infinity : Math.min(min.z, box.max[2] - box.min[2]),
- x: Math.min(min.x, box.max[0] - box.min[0]),
- y: Math.min(min.y, box.max[1] - box.min[1]),
- z: Math.min(min.z, box.max[2] - box.min[2]),
}),
{ x: Number.POSITIVE_INFINITY, y: Number.POSITIVE_INFINITY, z: Number.POSITIVE_INFINITY },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const minDimensions = userBoundingBoxes.reduce( | |
| (min, { boundingBox: box }) => ({ | |
| x: Math.min(min.x, box.max[0] - box.min[0]), | |
| y: Math.min(min.y, box.max[1] - box.min[1]), | |
| z: Math.min(min.z, box.max[2] - box.min[2]), | |
| }), | |
| { x: Number.POSITIVE_INFINITY, y: Number.POSITIVE_INFINITY, z: Number.POSITIVE_INFINITY }, | |
| ); | |
| const minDimensions = userBoundingBoxes.reduce( | |
| (min, { boundingBox: box }) => ({ | |
| // Validate that dimensions are positive | |
| x: box.max[0] <= box.min[0] ? Infinity : Math.min(min.x, box.max[0] - box.min[0]), | |
| y: box.max[1] <= box.min[1] ? Infinity : Math.min(min.y, box.max[1] - box.min[1]), | |
| z: box.max[2] <= box.min[2] ? Infinity : Math.min(min.z, box.max[2] - box.min[2]), | |
| }), | |
| { x: Number.POSITIVE_INFINITY, y: Number.POSITIVE_INFINITY, z: Number.POSITIVE_INFINITY }, | |
| ); |
…ax-train-model-bbox-requirements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
220-225: Consider adding type guard for annotation id access.The current type casting approach for accessing the annotation id could be made more type-safe.
Consider this safer approach:
- annotationId: "id" in annotation ? annotation.id : annotation.annotationId, + annotationId: isHybridTracing(annotation) ? annotation.id : annotation.annotationId,Where
isHybridTracingis a type guard:function isHybridTracing(annotation: HybridTracing | APIAnnotation): annotation is HybridTracing { return 'id' in annotation; }
492-494: Consider extracting magic numbers into named constants.The minimum dimension size of 10 Vx is used as a magic number. This value appears to be an important threshold for the training process.
Consider extracting it into a named constant:
+const MIN_BOUNDING_BOX_DIMENSION = 10; // Voxels + if (width < 10 || height < 10 || depth < 10) { - tooSmallBoxes.push({ boundingBox: arrayBox, name, annotationId }); + if (width < MIN_BOUNDING_BOX_DIMENSION || + height < MIN_BOUNDING_BOX_DIMENSION || + depth < MIN_BOUNDING_BOX_DIMENSION) { + tooSmallBoxes.push({ boundingBox: arrayBox, name, annotationId }); + }
476-483: Consider validating dimensions before reduction.The current implementation might attempt to calculate minimum dimensions even if some boxes have invalid (zero or negative) dimensions.
Consider adding validation before reduction:
+ // Validate dimensions are positive + const invalidDimensions = userBoundingBoxes.find(({ boundingBox: box }) => { + const width = box.max[0] - box.min[0]; + const height = box.max[1] - box.min[1]; + const depth = box.max[2] - box.min[2]; + return width <= 0 || height <= 0 || depth <= 0; + }); + + if (invalidDimensions) { + return { + hasBBoxErrors: true, + hasBBoxWarnings: false, + errors: ["All bounding box dimensions must be positive"], + warnings: [], + }; + } + const minDimensions = userBoundingBoxes.reduce(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
69-70: LGTM! Clear and informative warning message.
The updated message effectively communicates the relaxed requirements while still providing helpful guidance about bounding box dimensions.
Remove requirements of bounding boxes to have the same x/y dimension as well as the same dimensions overall for model training. Needs https://github.com/scalableminds/voxelytics/pull/3796.
URL of deployed dev instance (used for testing):
Steps to test:
yarn enable-jobs, open "AI Models" from the Administration pane. Trying to start a training using the one annotation as well as using the two annotations with differently sized bounding boxes should be allowed.(Please delete unneeded items, merge only when none are left open)