Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax bounding box requirements for model training #8222

Merged
merged 10 commits into from
Nov 28, 2024
Merged

Relax bounding box requirements for model training #8222

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52e612d
Remove requirements of bounding boxes to have the same x/y dimension …
daniel-wer Nov 7, 2024
735b15b
Merge branch 'master' of github.com:scalableminds/webknossos into rel…
daniel-wer Nov 20, 2024
985e1b7
update changelog
daniel-wer Nov 20, 2024
50b8072
Merge branch 'master' of github.com:scalableminds/webknossos into rel…
daniel-wer Nov 25, 2024
7d7945c
Show warnings if bounding boxes for CNN training are suboptimal. Make…
daniel-wer Nov 25, 2024
103dc38
Change map to forEach
daniel-wer Nov 25, 2024
10bc398
Merge branch 'master' of github.com:scalableminds/webknossos into rel…
daniel-wer Nov 26, 2024
177c890
Include bounding box name in warnings
daniel-wer Nov 26, 2024
221af60
Merge branch 'master' of github.com:scalableminds/webknossos into rel…
daniel-wer Nov 28, 2024
a02a36a
Merge branch 'master' into relax-train-model-bbox-requirements
daniel-wer Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Reading image files on datastore filesystem is now done asynchronously. [#8126](https://github.com/scalableminds/webknossos/pull/8126)
- Improved error messages for starting jobs on datasets from other organizations. [#8181](https://github.com/scalableminds/webknossos/pull/8181)
- Removed bounding box size restriction for inferral jobs for super users. [#8200](https://github.com/scalableminds/webknossos/pull/8200)
- 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. [#8222](https://github.com/scalableminds/webknossos/pull/8222)

### Fixed
- Fix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176)
Expand Down
23 changes: 2 additions & 21 deletions frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import _ from "lodash";
import BoundingBox from "oxalis/model/bucket_data_handling/bounding_box";
import { formatVoxels } from "libs/format_utils";
import * as Utils from "libs/utils";
import { V3 } from "libs/mjs";
import type { APIAnnotation, APIDataset, ServerVolumeTracing } from "types/api_flow_types";
import type { Vector3 } from "oxalis/constants";
import { serverVolumeToClientVolumeTracing } from "oxalis/model/reducers/volumetracing_reducer";
Expand Down Expand Up @@ -66,7 +65,7 @@ enum AiModelCategory {
const ExperimentalWarning = () => (
<Row style={{ display: "grid", marginBottom: 16 }}>
<Alert
message="Please note that this feature is experimental. All bounding boxes must be the same size, with equal width and height. 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. Ensure the size is not too small (we recommend at least 10 Vx per dimension) and choose boxes that represent the data well."
type="warning"
showIcon
/>
Expand Down Expand Up @@ -424,25 +423,7 @@ function areInvalidBoundingBoxesIncluded(userBoundingBoxes: UserBoundingBox[]):
invalidBBoxesReason: "At least one bounding box must be defined.",
};
}
const getSize = (bbox: UserBoundingBox) => V3.sub(bbox.boundingBox.max, bbox.boundingBox.min);

const size = getSize(userBoundingBoxes[0]);
// width must equal height
if (size[0] !== size[1]) {
return {
areSomeBBoxesInvalid: true,
invalidBBoxesReason: "The bounding box width must equal its height.",
};
}
// all bounding boxes must have the same size
const areSizesIdentical = userBoundingBoxes.every((bbox) => V3.isEqual(getSize(bbox), size));
if (areSizesIdentical) {
return { areSomeBBoxesInvalid: false, invalidBBoxesReason: null };
}
return {
areSomeBBoxesInvalid: true,
invalidBBoxesReason: "All bounding boxes must have the same size.",
};
return { areSomeBBoxesInvalid: false, invalidBBoxesReason: null };
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Validate minimum box size (10 Vx per dimension)
  2. Validate that box dimensions are multiples of the smallest box
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

function AnnotationsCsvInput({
Expand Down