add detection mask threshold to dataset app config #5369
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
Documentation and Community
|
tom-vx51
changed the title
![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 comments (1)
app/packages/looker/src/state.ts (1)
Line range hint
391-401: Add maskThreshold to DEFAULT_BASE_OPTIONS.The default options should include the new maskThreshold property to ensure consistent behavior.
coloring: { by: "field", points: true, pool: ["#000000"], scale: null, seed: 0, maskTargets: {}, defaultMaskTargets: null, + maskThreshold: 0, targets: ["#000000"], },
🧹 Nitpick comments (1)
docs/source/user_guide/using_datasets.rst (1)
480-505: Documentation could be enhanced with additional details.While the documentation clearly explains the basic usage, consider adding:
- The valid range of threshold values (e.g., 0-255 for uint8 masks)
- Visual examples showing the effect of different thresholds
- Performance implications, if any
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (10)
app/packages/looker/src/state.ts(1 hunks)app/packages/looker/src/worker/painter.ts(2 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/recoil/color.ts(1 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/types.ts(1 hunks)app/schema.graphql(1 hunks)docs/source/user_guide/using_datasets.rst(1 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/state/src/recoil/types.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/relay/src/fragments/datasetAppConfigFragment.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/color.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/painter.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/selectors.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (9)
app/packages/relay/src/fragments/datasetAppConfigFragment.ts (1)
14-14: LGTM! Clean addition of the maskThreshold field.The field is correctly added to the GraphQL fragment and follows the schema conventions.
app/packages/state/src/recoil/types.ts (1)
126-126: LGTM! Well-typed optional property addition.The
maskThresholdproperty is correctly typed as an optional number, maintaining consistency with the GraphQL schema.app/packages/state/src/recoil/color.ts (1)
59-59: LGTM! Proper integration with Recoil state management.The
maskThresholdproperty is correctly added to the coloring selector, maintaining consistency with the Recoil patterns and type definitions.app/packages/looker/src/state.ts (1)
28-28: LGTM! Clean interface extension.The
maskThresholdproperty is correctly added to the Coloring interface with proper typing.app/packages/looker/src/worker/painter.ts (1)
122-122: Implementation of mask threshold looks good!The code correctly:
- Initializes maskThreshold with a fallback to 0 using nullish coalescing
- Consistently applies the threshold check in both mask handling paths
Also applies to: 133-133, 140-140
app/packages/state/src/recoil/selectors.ts (1)
145-150: Well-structured selector implementation!The maskThreshold selector:
- Is correctly typed as number
- Uses appropriate default value handling
- Follows the established selector pattern
fiftyone/server/query.py (1)
221-221: Clean addition of mask_threshold field!The implementation:
- Uses correct integer type
- Maintains consistency with frontend default value
- Is properly placed in the DatasetAppConfig class
fiftyone/core/odm/dataset.py (1)
541-542: Excellent documentation and implementation!The changes:
- Provide clear documentation explaining the threshold's purpose and applicability
- Maintain consistent field definition with query.py
- Follow the established pattern for configuration options
Also applies to: 558-558
app/schema.graphql (1)
258-258: LGTM! The newmaskThresholdfield is well-defined.The field is appropriately typed as an optional
Intand follows GraphQL naming conventions. Its placement inDatasetAppConfigmakes sense as it's a dataset-specific visualization setting.
| @@ -553,6 +555,7 @@ class DatasetAppConfig(EmbeddedDocument): | |||
| sidebar_groups = ListField( | |||
| EmbeddedDocumentField(SidebarGroupDocument), default=None | |||
| ) | |||
| mask_threshold = IntField(default=0) | |||
There was a problem hiding this comment.
I think that the App gets all its data via dataset.app_config, which should make this comment moot.
But... just be aware that there's no database migration here, so if there were some code path that directly pulled a dataset's app config from the database, then this would be missing from the app config of pre-existing datasets.
There was a problem hiding this comment.
Good flag, I believe there's a guard on the app side against it:
...
return get(datasetAppConfig)?.maskThreshold ?? 0;
...
There was a problem hiding this comment.
Yeah confirmed that this works for existing datasets. The app does a nullish check and defaults to 0 if it's not contained in the config.
| @@ -477,6 +477,32 @@ You can disable frame filtering for a video dataset as follows: | |||
| Did you know? You can also globally disable frame filtering for all video | |||
| datasets via your :ref:`App config <configuring-fiftyone-app>`. | |||
|
|
|||
| .. _dataset-app-config-detection-mask-threshold: | |||
|
|
|||
| Configure detection mask threshold | |||
There was a problem hiding this comment.
I believe this only applies to instance segmentations, right? It would be nice to link to this section of the docs from the section that describes instance segmentations:
https://docs.voxel51.com/user_guide/using_datasets.html#instance-segmentations
Speaking of, note that the section on instance segmentations currently provides the following guidance:
Masks stored directly in the database must be 2D numpy arrays containing either booleans or 0/1 integers that encode the extent of the instance mask within the bounding_box of the object.
For masks stored on disk, the mask_path attribute should contain the file path to the mask image. We recommend storing masks as single-channel PNG images, where a pixel value of 0 indicates the background (rendered as transparent in the App), and any other value indicates the object.
Does that description need to be updated in light of this proposed change?
Also note that there is some connection between how masks are rendered in the App and the semantics of other operations, such as when evaluating instance segmentations:
fiftyone/fiftyone/utils/eval/detection.py
Lines 125 to 127 in 60971f8
If evaluate_detections() treats mask definitions differently than the App does, then it may be misleading to users why the quantitative evaluation metrics don't align with what they see in the App. This is tricky though, because app_config.mask_threshold is an App-centric configuration setting, while evaluate_detections() is an SDK concept.
There was a problem hiding this comment.
Update: after tracing down how evaluate_detections() handles instance masks, they are currently parsed specifically here:
https://github.com/voxel51/eta/blob/7280590da6b03a76d21d0c3d24aa2067b091985b/eta/core/image.py#L1089
@mwoodson1 is also actively working on an alternative implementation in #5283 where instance mask arithmetic would instead be implemented like this:
fiftyone/fiftyone/utils/iou.py
Lines 575 to 576 in 72d933b
So I believe the SDK is effectively hard-coded to mask_threshold=0 for the purposes of evaluating instance segmentations.
There was a problem hiding this comment.
Another place where instance segmentation semantics matter on the backend is Detections.to_segmentation(), which ends up treating all instance masks with mask_threshold=0 because of this line.
There was a problem hiding this comment.
Good notes, will update the docs.
If evaluate_detections() treats mask definitions differently than the App does, then it may be misleading to users why the quantitative evaluation metrics don't align with what they see in the App. This is tricky though, because app_config.mask_threshold is an App-centric configuration setting, while evaluate_detections() is an SDK concept.
This change was only meant to be a visual configuration, but I agree with your concern. I admittedly haven't tested this behavior in the SDK and was trusting the experience in this comment. Would you like any SDK changes as part of this PR? Or additional documentation? Or something else?
There was a problem hiding this comment.
Would you like any SDK changes as part of this PR? Or additional documentation? Or something else?
Depends on the motivation for the change. Can you share some additional context?
There was a problem hiding this comment.
If the user is only interested in visualizing but not evaluating data, then we could just ship this as-is after updating the documentation as mentioned so that its clear that dataset.app_config.mask_threshold is a visual-only change.
If the user is also interested in evaluating these instance segmentations, or if our primary motivation is internal development, then I'd want to understand a bit more.
The main use case I could imagine here would be that users are working with instance segmentation models that generate probabilistic masks that are represented initially as float values in [0, 1], which would then be converted to uint8(0, 255) for storage reasons. In this case, a user might want to evaluate different candidate thresholds for binarizing the masks. In this context, what I'd want is:
- Ability to visualize probabilistic instance masks as heatmaps (like our
Heatmaplabel type) - Ability to visualize different candidate
mask_thresholdvalues in the App via a slider or similar- Point 1 could be a special case of this where the mask threshold is disabled
- Ability to define an appropriate
mask_thresholdfor instance segmentation field(s), which would then be used subsequently by the App and via the SDK- As Ben mentions, the most natural way to support that would be
dataset.default_instance_mask_thresholdanddataset.instance_mask_thresholds, for consistency with how mask targets are stored today
- As Ben mentions, the most natural way to support that would be
We could choose to not build Point 3, instead recommending to the user to use 1 and 2 to explore different thresholds, and then once you've picked one, binarize your instance masks and store these on your FO dataset for downstream tasks like evaluation.
|
I do think considering the pattern established by For example, |
What changes are proposed in this pull request?
This PR adds a configurable detection mask threshold to the dataset app config. For masks containing non-binary values (e.g. a uint8 mask with values in [0,255]), FiftyOne currently treats all non-zero elements as masked; this effectively acts as a default mask threshold of 0. This change allows for setting a custom threshold, which causes masks to render only for mask values exceeding the threshold.
How is this patch tested? If it is not, please explain why.
Tested via local app development. Modifying
dataset.app_config.mask_thresholdresults in conditional masking based on the configured threshold (default = 0).Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
Add support for configuring mask thresholds for non-binary detection masks.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Documentation
Technical Improvements
maskThresholdacross multiple components and interfaces