Choose mag when training models on multiple annotations #8266
Conversation
📝 WalkthroughWalkthroughThis pull request updates the WEBKNOSSOS application with several user-facing changes, including the introduction of a magnification selection feature for AI model training. The term "resolution" has been replaced with "magnification" throughout the codebase. Enhancements to layer naming and dataset management have been implemented, along with performance optimizations for data loading. The bounding box tool has been improved for better user feedback, and multiple bugs have been fixed. Overall, these changes enhance the functionality and usability of the application. Changes
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)
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: 2
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (5)
136-138: Remove unused parameter ingetMagsForSegmentationLayerThe parameter
_annotationIdis not used within thegetMagsForSegmentationLayerfunction. Consider removing it to improve code clarity.
169-169: Address TODO: RenamegetMagsForSegmentationLayermethodThere's a TODO comment indicating the need to rename the
getMagsForSegmentationLayermethod. Please rename the method to reflect its functionality accurately.If you need assistance choosing an appropriate name, I can help suggest one.
186-186: Removeconsole.logstatementThe
console.logstatement is useful for debugging but should be removed before production to avoid unnecessary console output.
414-414: Removeconsole.logstatementThe
console.log(layer);statement should be removed or replaced with proper logging if necessary.
417-417: Address TODO: Display fallback layers correctlyThere's a TODO comment to fix the display of fallback layers with the correct name. Implementing this will enhance the user interface and reduce confusion.
I can assist with implementing this feature if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.unreleased.md(1 hunks)conf/application.conf(1 hunks)frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(3 hunks)frontend/javascripts/components/layer_selection.tsx(3 hunks)frontend/javascripts/components/mag_selection.tsx(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🔇 Additional comments (5)
frontend/javascripts/components/layer_selection.tsx (1)
11-11: Enhancement: Added onChange prop to LayerSelection components
The addition of the optional onChange prop increases the flexibility of the LayerSelection and LayerSelectionFormItem components, allowing parent components to respond to selection changes.
Also applies to: 69-69, 90-90
CHANGELOG.unreleased.md (1)
16-16: Changelog updated with new feature
The addition to the changelog accurately reflects the new functionality of selecting magnifications when training AI models.
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (2)
Line range hint 112-137: LGTM! The magnification retrieval logic is well-implemented.
The implementation correctly handles both annotation layers and segmentation layers, with proper error handling and clear separation of concerns. The use of getMagInfo ensures consistent magnification information format.
155-155: LGTM! Prop name updated consistently.
The prop name update matches the function rename, maintaining consistency throughout the component.
conf/application.conf (1)
154-155: Verify if enabling these features by default is intended.
Both jobsEnabled and voxelyticsEnabled are set to true in the default configuration. While these features are necessary for AI model training functionality:
- The comment above suggests using "yarn enable-jobs" for local development
- These features might have infrastructure dependencies that need to be properly configured first
Let's check if there are any related configuration or setup instructions:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
391-399: Consider debouncing the onChange handler.The
onChangehandler triggers state updates and form field changes which could lead to performance issues if the user changes layers rapidly.+const debouncedSetIntersectingMags = _.debounce(setIntersectingMags, 300); onChange={() => { - setIntersectingMags( + debouncedSetIntersectingMags( annotationId, dataset, form.getFieldValue(["trainingAnnotations", idx, "layerName"]), form.getFieldValue(["trainingAnnotations", idx, "imageDataLayer"]), ); form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined); }}
410-411: Address TODO comment regarding fallback layer names.There's a TODO comment about fixing the display of fallback layer names. This should be addressed to improve UX.
Would you like me to help implement the fallback layer name formatting or create a GitHub issue to track this task?
427-434: Consider memoizing magInfo calculation.The magInfo prop calculation could be memoized to prevent unnecessary recalculations during renders.
+const getMemoizedMagInfo = React.useMemo(() => { + if (mags != null) { + return mags.find((magInfoPerAnno) => magInfoPerAnno.annotationId === annotationId)?.magInfo; + } + return initialMagInfo; +}, [mags, annotationId, initialMagInfo]); <MagSelectionFormItem name={["trainingAnnotations", idx, "mag"]} - magInfo={ - mags != null - ? mags.find((magInfoPerAnno) => magInfoPerAnno.annotationId === annotationId) - ?.magInfo - : initialMagInfo - } + magInfo={getMemoizedMagInfo} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/components/mag_selection.tsx(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)
- frontend/javascripts/components/mag_selection.tsx
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (2)
37-48: LGTM: Clean type imports and new component imports.
The imports are well-organized and the new imports for MagSelectionFormItem and MagInfo are properly added.
180-196: LGTM: Well-implemented intersection logic for magnification lists.
The getIntersectingMagList function correctly handles the intersection of magnifications between data and ground truth layers.
There was a problem hiding this comment.
sorry, I started reviewing the PR a few days ago and then apparently something else grabbed my attention. I finished the review now and left a few comments.
It's cool that the mag selection seems to work 👍 however, I find the code a bit confusing. I think my main gripe with it is that mags can be null and that it is written only on certain events. In my mind, all information should be available at all times and mags could always be computed on demand (memoized if necessary). I understand that antd forms can be annoying, so maybe it doesn't work how I imagine it, but please let's have a call to discuss this :)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
180-196: Consider using more functional programming patterns.The function could be more concise using functional programming patterns.
- return groundTruthLayerMags?.filter((groundTruthMag) => - dataLayerMags?.find((mag) => V3.equals(mag, groundTruthMag)), - ); + return groundTruthLayerMags?.filter((groundTruthMag) => + dataLayerMags?.some((mag) => V3.equals(mag, groundTruthMag)) + );
198-219: Simplify state handling logic.The state handling in getIntersectingMags could be simplified using immutable patterns.
- if (mags == null) { - return [{ annotationId, magInfo: new MagInfo(intersectingMags) }]; - } else { - return mags.map((mag) => - mag.annotationId === annotationId - ? { annotationId, magInfo: new MagInfo(intersectingMags) } - : mag, - ); - } + const newMagInfo = { annotationId, magInfo: new MagInfo(intersectingMags) }; + return mags?.map(mag => + mag.annotationId === annotationId ? newMagInfo : mag + ) ?? [newMagInfo];
412-413: Address the TODO comment about fallback layer names.The comment indicates a need to improve the display of fallback layer names.
Would you like me to help implement the fallback layer name formatting or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(6 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
137-139: Add error handling for missing segmentation layer.
The function assumes the segmentation layer will always be found.
161-161: LGTM: Clear and purposeful type definition.
The type effectively combines annotation ID with magnification information.
229-232: Add null check for layer resolutions.
The getMagsForColorLayer function should validate that the layer has resolutions before accessing them.
420-428: LGTM: Well-structured form component integration.
The MagSelectionFormItem integration is clean and handles both initial and updated states correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
185-185: Remove console.log statement.Debug logging should not be present in production code.
- console.log("getIntersectingMags");
337-339: Extract common layer change handler.The same layer change handler is duplicated across color layer and ground truth layer selections. Consider extracting it to a shared function.
+const handleLayerChange = (form: FormInstance, idx: number) => { + form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined); +}; const onChangeLayer = () => { - form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined); + handleLayerChange(form, idx); };Also applies to: 371-371, 387-388
382-383: Address the TODO comment about fallback layer names.The TODO indicates that fallback layers need better naming representation in the UI. This could impact user experience as it may be difficult for users to identify the correct layer.
Would you like me to help implement a solution for displaying fallback layer names in a more user-friendly format? I can help create a function that formats the layer names appropriately or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(5 hunks)
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
137-139:
Add error handling for missing segmentation layer.
The function assumes the segmentation layer will always be found. Consider adding error handling for cases where the layer might not exist.
const getMagsForSegmentationLayer = (_annotationId: string, layerName: string) => {
const segmentationLayer = getSegmentationLayerByHumanReadableName(dataset, tracing, layerName);
+ if (!segmentationLayer) {
+ throw new Error(`Segmentation layer "${layerName}" not found`);
+ }
return getMagInfo(segmentationLayer.resolutions);
};Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
208-224: Consider memoizing the intersection calculation.The
getIntersectingMagListfunction performs filtering operations that could benefit from memoization to prevent unnecessary recalculations.Consider wrapping the function with
useMemo:-const getIntersectingMagList = ( +const getIntersectingMagList = React.useMemo(() => ( annotationId: string, dataset: APIDataset, groundTruthLayerName: string, imageDataLayerName: string, ) => { const colorLayers = getColorLayers(dataset); const dataLayerMags = getMagsForColorLayer(colorLayers, imageDataLayerName); const groundTruthLayerMags = getMagsForSegmentationLayer( annotationId, groundTruthLayerName, ).getMagList(); return groundTruthLayerMags?.filter((groundTruthMag) => dataLayerMags?.find((mag) => V3.equals(mag, groundTruthMag)), ); -}; +}, [dataset, getMagsForSegmentationLayer]);
385-386: Address TODO comment regarding fallback layers.There's a TODO comment about fixing the display of fallback layer names.
Would you like me to help implement the fallback layer name formatting? I can propose a solution that includes the active layer status in the display name.
394-401: Simplify conditional rendering of magInfo.The conditional rendering of
magInfocan be simplified using nullish coalescing.Apply this diff:
- magInfo={ - magInfoPerLayer != null && magInfoPerLayer[idx] != null - ? magInfoPerLayer[idx] - : new MagInfo([]) - } + magInfo={magInfoPerLayer?.[idx] ?? new MagInfo([])}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(5 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
37-49: LGTM: Import statements and type definitions are well-organized.
The new imports for MagSelectionFormItem and MagInfo are properly added, and the API types are correctly imported.
340-342: 🛠️ Refactor suggestion
Extract duplicate onChange handler.
The onChangeLayer handler is duplicated in multiple places. Consider extracting it to a reusable function.
Apply this diff:
+const createLayerChangeHandler = (form: FormInstance, idx: number) => () => {
+ form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined);
+};
-const onChangeLayer = () => {
- form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined);
-};
+const onChangeLayer = createLayerChangeHandler(form, idx);Likely invalid or redundant comment.
226-229:
Add null check for layer resolutions.
The getMagsForColorLayer function should validate that the layer has resolutions before accessing them.
Apply this diff:
const getMagsForColorLayer = (colorLayers: APIDataLayer[], layerName: string) => {
const colorLayer = colorLayers.find((layer) => layer.name === layerName);
- return colorLayer != null ? getMagInfo(colorLayer.resolutions).getMagList() : null;
+ return colorLayer?.resolutions ? getMagInfo(colorLayer.resolutions).getMagList() : null;
};Likely invalid or redundant comment.
137-139:
Add error handling for missing segmentation layer.
The function assumes the segmentation layer will always be found. Consider adding error handling for cases where the layer might not exist.
Apply this diff:
const getMagsForSegmentationLayer = (_annotationId: string, layerName: string) => {
const segmentationLayer = getSegmentationLayerByHumanReadableName(dataset, tracing, layerName);
+ if (!segmentationLayer) {
+ throw new Error(`Segmentation layer "${layerName}" not found`);
+ }
return getMagInfo(segmentationLayer.resolutions);
};Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
183-207: Consider extracting the watcher logic into a custom hook.The watcher logic is complex and could be more maintainable if extracted into a custom hook. This would improve reusability and make the component's logic clearer.
Consider creating a custom hook like this:
function useMagInfoWatcher(form: FormInstance, annotationInfos: AnnotationInfoForAIJob[]) { const watcherFn = useCallback(() => { const getIntersectingMags = (idx: number, annotationId: string, dataset: APIDataset) => { const segmentationLayerName = form.getFieldValue(["trainingAnnotations", idx, "layerName"]); const imageDataLayerName = form.getFieldValue(["trainingAnnotations", idx, "imageDataLayer"]); if (segmentationLayerName != null && imageDataLayerName != null) { return new MagInfo( getIntersectingMagList(annotationId, dataset, segmentationLayerName, imageDataLayerName), ); } return new MagInfo([]); }; return annotationInfos.map((annotationInfo, idx: number) => { const annotation = annotationInfo.annotation; const annotationId = "id" in annotation ? annotation.id : annotation.annotationId; return getIntersectingMags(idx, annotationId, annotationInfo.dataset); }); }, [form, annotationInfos]); return Form.useWatch(watcherFn, form); }
386-387: Address the TODO comment about fallback layer names.The comment indicates that fallback layers need proper naming in the UI. This should be addressed to improve user experience.
Would you like me to help implement the fallback layer naming logic or create a GitHub issue to track this task?
341-343: Extract duplicate onChange handler logic.The same onChange handler logic is duplicated. Consider extracting it into a shared function.
Apply this diff to remove the duplication:
+const createLayerChangeHandler = (form: FormInstance, idx: number) => () => { + form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined); +}; -const onChangeLayer = () => { - form.setFieldValue(["trainingAnnotations", idx, "mag"], undefined); -}; <LayerSelection layers={colorLayers} getReadableNameForLayer={(layer) => layer.name} fixedLayerName={fixedSelectedColorLayer?.name || undefined} style={{ width: "100%" }} - onChange={onChangeLayer} + onChange={createLayerChangeHandler(form, idx)} /> <LayerSelectionFormItem name={["trainingAnnotations", idx, "layerName"]} chooseSegmentationLayer layers={segmentationLayers} getReadableNameForLayer={(layer) => layer.name} fixedLayerName={fixedSelectedSegmentationLayer?.name || undefined} label="Ground Truth Layer" - onChange={onChangeLayer} + onChange={createLayerChangeHandler(form, idx)} />Also applies to: 391-391
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
Line range hint 501-600: LGTM! Comprehensive validation logic.
The validation functions for annotations and bounding boxes are well-implemented with thorough checks and clear error messages.
229-232:
Add null check for layer resolutions.
The getMagsForColorLayer function should validate that the layer has resolutions before accessing them.
Apply this diff to add validation:
const getMagsForColorLayer = (colorLayers: APIDataLayer[], layerName: string) => {
const colorLayer = colorLayers.find((layer) => layer.name === layerName);
- return colorLayer != null ? getMagInfo(colorLayer.resolutions).getMagList() : null;
+ return colorLayer?.resolutions ? getMagInfo(colorLayer.resolutions).getMagList() : null;
};Likely invalid or redundant comment.
137-139:
Add error handling for missing segmentation layer.
The function assumes the segmentation layer will always be found. Consider adding error handling for cases where the layer might not exist.
Apply this diff to add error handling:
const getMagsForSegmentationLayer = (_annotationId: string, layerName: string) => {
const segmentationLayer = getSegmentationLayerByHumanReadableName(dataset, tracing, layerName);
+ if (!segmentationLayer) {
+ throw new Error(`Segmentation layer "${layerName}" not found`);
+ }
return getMagInfo(segmentationLayer.resolutions);
};Likely invalid or redundant comment.
|
I think I addressed all the feedback for now 🤞 From my side an open question that is left is how to handle volume annotation layer names (=tracing ids from what I understand) in the selection. Maybe something like this? |
…n-readable layer names should be used instead
URL of deployed dev instance (used for testing):
Steps to test:
Enable jobs
Open an annotation and go to AI Analysis > Train a model
Select a ground truth and a color layer
Select a mag. Check that all available mags are present in both layers
Start a training / check network tab that the right mag was sent in the request
Do the same thing, but for multiple annotations. Do that by navigating to Administration > AI models > Train new model.
TODOs:
update mags after any of the input layers change
find union of data and ground truth layer
check that it works from within annotation and with multiple annotations
fix spacing (required vs not required fields in form, see pic)
address first review
regarding testing: does this need to be tested after the request was sent? if yes, how?
when opening the modal, there is a ground truth layer option for the active segmentation layer, and for the fallback layer. the first one's name is simply the tracing id, which makes it harder to understand what is happening.
Issues:
(Please delete unneeded items, merge only when none are left open)