Add DetectionsMask label class #4500
Conversation
WalkthroughThe latest update enhances the FiftyOne tool by integrating a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FiftyOne
participant Label
participant DetectionsMask
User->>FiftyOne: Request to handle detection mask
FiftyOne->>Label: Convert to DetectionsMask
Label->>DetectionsMask: Create instance
DetectionsMask->>Label: Return mask representation
Label->>FiftyOne: Pass mask data
FiftyOne->>User: Provide detection mask info
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Outside diff range and nitpick comments (12)
tests/unittests/label_tests.py (2)
Line range hint
53-53: Usekey in dictinstead ofkey in dict.keys()for better performance and readability.- if key in dict.keys(): + if key in dict:
Line range hint
623-623: Remove assignments to unused variablesid1andid4to clean up the code and avoid unnecessary memory usage.- id1 = detections[0].id - id4 = detections[3].idAlso applies to: 626-626
fiftyone/core/labels.py (10)
Line range hint
39-39: Remove unnecessary inheritance fromobject.- class _NoDefault(object): + class _NoDefault:
Line range hint
100-100: Use f-string for formatting.- "%s has no attribute '%s'" % (self.__class__.__name__, name) + f"{self.__class__.__name__} has no attribute '{name}'"
Line range hint
126-128: When re-raising exceptions, useraise ... from Noneto avoid chaining exceptions that are unrelated.- raise AttributeError( - "%s has no attribute '%s'" % (self.__class__.__name__, name) - ) + raise AttributeError( + f"{self.__class__.__name__} has no attribute '{name}'" + ) from None
Line range hint
268-268: Use f-string for formatting.- "%s has no attribute '%s'" % (self.__class__.__name__, name) + f"{self.__class__.__name__} has no attribute '{name}'"
Line range hint
307-310: When re-raising exceptions, useraise ... from Noneto avoid chaining exceptions that are unrelated.- raise AttributeError( - "%s has no attribute '%s'" % (self.__class__.__name__, name) - ) + raise AttributeError( + f"{self.__class__.__name__} has no attribute '{name}'" + ) from None
Line range hint
343-343: Remove unnecessary inheritance fromobject.- class _HasLabelList(object): + class _HasLabelList:
Line range hint
1015-1018: Use a ternary operator to simplify the assignment.- if filled is not None: - _filled = filled - else: - _filled = self.filled + _filled = filled if filled is not None else self.filled
Line range hint
1892-1892: Remove unnecessary parentheses.- if (target is not None and not is_rgb and target > 255): + if target is not None and not is_rgb and target > 255:
Line range hint
1897-1900: Use a ternary operator to simplify the assignment.- if target is not None: - is_rgb = fof.is_rgb_target(target) - else: - is_rgb = False + is_rgb = fof.is_rgb_target(target) if target is not None else False
Line range hint
1906-1909: Use a ternary operator to simplify the assignment.- if target is not None and not is_rgb and target > 255: - dtype = int - else: - dtype = np.uint8 + dtype = int if target is not None and not is_rgb and target > 255 else np.uint8
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- fiftyone/core/labels.py (18 hunks)
- tests/unittests/label_tests.py (1 hunks)
Additional context used
Ruff
tests/unittests/label_tests.py
53-53: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
623-623: Local variable
id1is assigned to but never used (F841)Remove assignment to unused variable
id1
626-626: Local variable
id4is assigned to but never used (F841)Remove assignment to unused variable
id4fiftyone/core/labels.py
39-39: Class
_NoDefaultinherits fromobject(UP004)Remove
objectinheritance
100-100: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
126-128: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
127-127: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
268-268: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
307-310: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
308-309: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
315-318: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
316-317: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
343-343: Class
_HasLabelListinherits fromobject(UP004)Remove
objectinheritance
583-583: No explicit
stacklevelkeyword argument found (B028)
635-635: No explicit
stacklevelkeyword argument found (B028)
650-650: Class
_HasMediainherits fromobject(UP004)Remove
objectinheritance
1015-1018: Use ternary operator
_filled = filled if filled is not None else self.filledinstead ofif-else-block (SIM108)Replace
if-else-block with_filled = filled if filled is not None else self.filled
1892-1892: Avoid extraneous parentheses (UP034)
Remove extraneous parentheses
1897-1900: Use ternary operator
is_rgb = fof.is_rgb_target(target) if target is not None else Falseinstead ofif-else-block (SIM108)Replace
if-else-block withis_rgb = fof.is_rgb_target(target) if target is not None else False
1906-1909: Use ternary operator
dtype = int if target is not None and not is_rgb and target > 255 else np.uint8instead ofif-else-block (SIM108)Replace
if-else-block withdtype = int if target is not None and not is_rgb and target > 255 else np.uint8
1924-1927: Use ternary operator
is_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)instead ofif-else-block (SIM108)Replace
if-else-block withis_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)
2121-2121: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2135-2135: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2254-2255: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2264-2265: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2274-2275: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
Additional comments not posted (2)
tests/unittests/label_tests.py (2)
473-506: The new test cases forto_detections_maskand related conversions are comprehensive and cover various scenarios including conversions between different mask types and label types. This is crucial for ensuring that the new functionality behaves as expected across different use cases.
553-573: The tests for_parse_stuff_instanceand_parse_thing_instancemethods are well-implemented and check both the bounding box calculations and the correctness of the generated instance masks. These tests are essential for validating the changes made in the PR to fix the off-by-one error mentioned.
|
Hi @kemaleren! 👋 This code looks great! Thank you for the thorough description 🙇 Others may chime in, but this addition makes sense to me. Note that CVPR is going on this week, so we will be short on cycles. But next week I will look into App support |
kemaleren
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (14)
setup.py (2)
Line range hint
27-28: Replace percent formatting with modern format specifiers for better readability and performance.- "Release version does not match version: %s and %s" % (version, VERSION) + f"Release version does not match version: {version} and {VERSION}"
Line range hint
122-122: Theopenfunction's mode parameter is redundant here as the default mode is 'r' (read), which is what is being used.- with open("README.md", "r") as fh: + with open("README.md") as fh:tests/unittests/label_tests.py (2)
Line range hint
55-55: Refactor the check for dictionary keys to use a more Pythonic approach.- if "key" in dict.keys(): + if "key" in dict:
Line range hint
658-658: Variablesid1andid4are assigned but never used within the scope of the function. Consider removing these if they are not needed.- id1 = detections[0].id - id4 = detections[3].idAlso applies to: 661-661
fiftyone/core/labels.py (10)
Line range hint
39-39: Remove unnecessary inheritance fromobject.- class _NoDefault(object): + class _NoDefault:
Line range hint
100-100: Use format specifiers instead of percent format for string formatting.- "%s has no attribute '%s'" % (self.__class__.__name__, name) + "{} has no attribute '{}'".format(self.__class__.__name__, name)
Line range hint
126-128: When re-raising exceptions, useraise ... from errorraise ... from Noneto distinguish them from errors in exception handling.- raise AttributeError( - "%s has no attribute '%s'" % (self.__class__.__name__, name) - ) + raise AttributeError( + "{} has no attribute '{}'".format(self.__class__.__name__, name) + ) from None
Line range hint
268-268: Use format specifiers for string formatting.- "%s has no attribute '%s'" % (self.__class__.__name__, name) + "{} has no attribute '{}'".format(self.__class__.__name__, name)
Line range hint
307-310: When re-raising exceptions, useraise ... from errto provide more context about the original exception.- raise AttributeError( - "%s has no attribute '%s'" % (self.__class__.__name__, name) - ) + raise AttributeError( + "{} has no attribute '{}'".format(self.__class__.__name__, name) + ) from err
Line range hint
343-343: Remove unnecessary inheritance fromobject.- class _HasLabelList(object): + class _HasLabelList:
Line range hint
1015-1018: Use a ternary operator to simplify the assignment.- if filled is not None: - _filled = filled - else: - _filled = self.filled + _filled = filled if filled is not None else self.filled
Line range hint
1892-1892: Avoid extraneous parentheses.- if (target is not None and not is_rgb and target > 255): + if target is not None and not is_rgb and target > 255:
Line range hint
1897-1900: Use a ternary operator to simplify the assignment.- if target is not None: - is_rgb = fof.is_rgb_target(target) - else: - is_rgb = False + is_rgb = fof.is_rgb_target(target) if target is not None else False
Line range hint
1906-1909: Use a ternary operator to simplify the assignment.- if target is not None and not is_rgb and target > 255: - dtype = int - else: - dtype = np.uint8 + dtype = int if target is not None and not is_rgb and target > 255 else np.uint8
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- fiftyone/public.py (1 hunks)
- fiftyone/core/labels.py (18 hunks)
- setup.py (1 hunks)
- tests/unittests/label_tests.py (3 hunks)
Additional context used
Ruff
setup.py
27-28: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
122-122: Unnecessary open mode parameters (UP015)
Remove open mode parameters
fiftyone/__public__.py
16-31: Module level import not at top of file (E402)
17-17:
.core.aggregations.Aggregationimported but unused (F401)Remove unused import
18-18:
.core.aggregations.Boundsimported but unused (F401)Remove unused import
19-19:
.core.aggregations.Countimported but unused (F401)Remove unused import
20-20:
.core.aggregations.CountValuesimported but unused (F401)Remove unused import
21-21:
.core.aggregations.Distinctimported but unused (F401)Remove unused import
22-22:
.core.aggregations.FacetAggregationsimported but unused (F401)Remove unused import
23-23:
.core.aggregations.HistogramValuesimported but unused (F401)Remove unused import
24-24:
.core.aggregations.Meanimported but unused (F401)Remove unused import
25-25:
.core.aggregations.Quantilesimported but unused (F401)Remove unused import
26-26:
.core.aggregations.Schemaimported but unused (F401)Remove unused import
27-27:
.core.aggregations.ListSchemaimported but unused (F401)Remove unused import
28-28:
.core.aggregations.Stdimported but unused (F401)Remove unused import
29-29:
.core.aggregations.Sumimported but unused (F401)Remove unused import
30-30:
.core.aggregations.Valuesimported but unused (F401)Remove unused import
32-32: Module level import not at top of file (E402)
32-32:
.core.collections.SaveContextimported but unused (F401)Remove unused import:
.core.collections.SaveContext
33-33: Module level import not at top of file (E402)
33-33:
.core.config.AppConfigimported but unused (F401)Remove unused import:
.core.config.AppConfig
34-45: Module level import not at top of file (E402)
35-35:
.core.dataset.Datasetimported but unused (F401)Remove unused import
36-36:
.core.dataset.list_datasetsimported but unused (F401)Remove unused import
37-37:
.core.dataset.dataset_existsimported but unused (F401)Remove unused import
38-38:
.core.dataset.load_datasetimported but unused (F401)Remove unused import
39-39:
.core.dataset.delete_datasetimported but unused (F401)Remove unused import
40-40:
.core.dataset.delete_datasetsimported but unused (F401)Remove unused import
41-41:
.core.dataset.delete_non_persistent_datasetsimported but unused (F401)Remove unused import
42-42:
.core.dataset.get_default_dataset_nameimported but unused (F401)Remove unused import
43-43:
.core.dataset.make_unique_dataset_nameimported but unused (F401)Remove unused import
44-44:
.core.dataset.get_default_dataset_dirimported but unused (F401)Remove unused import
46-50: Module level import not at top of file (E402)
47-47:
.core.expressions.ViewFieldimported but unused (F401)Remove unused import
48-48:
.core.expressions.ViewExpressionimported but unused (F401)Remove unused import
49-49:
.core.expressions.VALUEimported but unused (F401)Remove unused import
51-81: Module level import not at top of file (E402)
52-52:
.core.fields.flatten_schemaimported but unused (F401)Remove unused import
53-53:
.core.fields.ArrayFieldimported but unused (F401)Remove unused import
54-54:
.core.fields.BooleanFieldimported but unused (F401)Remove unused import
55-55:
.core.fields.ClassesFieldimported but unused (F401)Remove unused import
56-56:
.core.fields.ColorFieldimported but unused (F401)Remove unused import
57-57:
.core.fields.DateFieldimported but unused (F401)Remove unused import
58-58:
.core.fields.DateTimeFieldimported but unused (F401)Remove unused import
59-59:
.core.fields.DictFieldimported but unused (F401)Remove unused import
60-60:
.core.fields.EmbeddedDocumentFieldimported but unused (F401)Remove unused import
61-61:
.core.fields.EmbeddedDocumentListFieldimported but unused (F401)Remove unused import
62-62:
.core.fields.Fieldimported but unused (F401)Remove unused import
63-63:
.core.fields.FrameNumberFieldimported but unused (F401)Remove unused import
64-64:
.core.fields.FrameSupportFieldimported but unused (F401)Remove unused import
65-65:
.core.fields.FloatFieldimported but unused (F401)Remove unused import
66-66:
.core.fields.GeoPointFieldimported but unused (F401)Remove unused import
67-67:
.core.fields.GeoLineStringFieldimported but unused (F401)Remove unused import
68-68:
.core.fields.GeoPolygonFieldimported but unused (F401)Remove unused import
69-69:
.core.fields.GeoMultiPointFieldimported but unused (F401)Remove unused import
70-70:
.core.fields.GeoMultiLineStringFieldimported but unused (F401)Remove unused import
71-71:
.core.fields.GeoMultiPolygonFieldimported but unused (F401)Remove unused import
72-72:
.core.fields.IntFieldimported but unused (F401)Remove unused import
73-73:
.core.fields.KeypointsFieldimported but unused (F401)Remove unused import
74-74:
.core.fields.ListFieldimported but unused (F401)Remove unused import
75-75:
.core.fields.ObjectIdFieldimported but unused (F401)Remove unused import
76-76:
.core.fields.PolylinePointsFieldimported but unused (F401)Remove unused import
77-77:
.core.fields.ReferenceFieldimported but unused (F401)Remove unused import
78-78:
.core.fields.StringFieldimported but unused (F401)Remove unused import
79-79:
.core.fields.MaskTargetsFieldimported but unused (F401)Remove unused import
80-80:
.core.fields.VectorFieldimported but unused (F401)Remove unused import
82-82: Module level import not at top of file (E402)
82-82:
.core.frame.Frameimported but unused (F401)Remove unused import:
.core.frame.Frame
83-83: Module level import not at top of file (E402)
83-83:
.core.groups.Groupimported but unused (F401)Remove unused import:
.core.groups.Group
84-107: Module level import not at top of file (E402)
85-85:
.core.labels.Labelimported but unused (F401)Remove unused import
86-86:
.core.labels.Attributeimported but unused (F401)Remove unused import
87-87:
.core.labels.BooleanAttributeimported but unused (F401)Remove unused import
88-88:
.core.labels.CategoricalAttributeimported but unused (F401)Remove unused import
89-89:
.core.labels.NumericAttributeimported but unused (F401)Remove unused import
90-90:
.core.labels.ListAttributeimported but unused (F401)Remove unused import
91-91:
.core.labels.Regressionimported but unused (F401)Remove unused import
92-92:
.core.labels.Classificationimported but unused (F401)Remove unused import
93-93:
.core.labels.Classificationsimported but unused (F401)Remove unused import
94-94:
.core.labels.Detectionimported but unused (F401)Remove unused import
95-95:
.core.labels.Detectionsimported but unused (F401)Remove unused import
96-96:
.core.labels.DetectionsMaskimported but unused (F401)Remove unused import
97-97:
.core.labels.Polylineimported but unused (F401)Remove unused import
98-98:
.core.labels.Polylinesimported but unused (F401)Remove unused import
99-99:
.core.labels.Keypointimported but unused (F401)Remove unused import
100-100:
.core.labels.Keypointsimported but unused (F401)Remove unused import
101-101:
.core.labels.Segmentationimported but unused (F401)Remove unused import
102-102:
.core.labels.Heatmapimported but unused (F401)Remove unused import
103-103:
.core.labels.TemporalDetectionimported but unused (F401)Remove unused import
104-104:
.core.labels.TemporalDetectionsimported but unused (F401)Remove unused import
105-105:
.core.labels.GeoLocationimported but unused (F401)Remove unused import
106-106:
.core.labels.GeoLocationsimported but unused (F401)Remove unused import
108-111: Module level import not at top of file (E402)
109-109:
.core.logging.get_logging_levelimported but unused (F401)Remove unused import
110-110:
.core.logging.set_logging_levelimported but unused (F401)Remove unused import
112-116: Module level import not at top of file (E402)
113-113:
.core.metadata.Metadataimported but unused (F401)Remove unused import
114-114:
.core.metadata.ImageMetadataimported but unused (F401)Remove unused import
115-115:
.core.metadata.VideoMetadataimported but unused (F401)Remove unused import
117-128: Module level import not at top of file (E402)
118-118:
.core.models.apply_modelimported but unused (F401)Remove unused import
119-119:
.core.models.compute_embeddingsimported but unused (F401)Remove unused import
120-120:
.core.models.compute_patch_embeddingsimported but unused (F401)Remove unused import
121-121:
.core.models.load_modelimported but unused (F401)Remove unused import
122-122:
.core.models.Modelimported but unused (F401)Remove unused import
123-123:
.core.models.ModelConfigimported but unused (F401)Remove unused import
124-124:
.core.models.EmbeddingsMixinimported but unused (F401)Remove unused import
125-125:
.core.models.TorchModelMixinimported but unused (F401)Remove unused import
126-126:
.core.models.ModelManagerConfigimported but unused (F401)Remove unused import
127-127:
.core.models.ModelManagerimported but unused (F401)Remove unused import
129-138: Module level import not at top of file (E402)
130-130:
.core.odm.ColorSchemeimported but unused (F401)Remove unused import
131-131:
.core.odm.DatasetAppConfigimported but unused (F401)Remove unused import
132-132:
.core.odm.DynamicEmbeddedDocumentimported but unused (F401)Remove unused import
133-133:
.core.odm.EmbeddedDocumentimported but unused (F401)Remove unused import
134-134:
.core.odm.KeypointSkeletonimported but unused (F401)Remove unused import
135-135:
.core.odm.Panelimported but unused (F401)Remove unused import
136-136:
.core.odm.SidebarGroupDocumentimported but unused (F401)Remove unused import
137-137:
.core.odm.Spaceimported but unused (F401)Remove unused import
139-154: Module level import not at top of file (E402)
140-140:
.core.plots.plot_confusion_matriximported but unused (F401)Remove unused import
141-141:
.core.plots.plot_pr_curveimported but unused (F401)Remove unused import
142-142:
.core.plots.plot_pr_curvesimported but unused (F401)Remove unused import
143-143:
.core.plots.plot_roc_curveimported but unused (F401)Remove unused import
144-144:
.core.plots.linesimported but unused (F401)Remove unused import
145-145:
.core.plots.scatterplotimported but unused (F401)Remove unused import
146-146:
.core.plots.location_scatterplotimported but unused (F401)Remove unused import
147-147:
.core.plots.Plotimported but unused (F401)Remove unused import
148-148:
.core.plots.ResponsivePlotimported but unused (F401)Remove unused import
149-149:
.core.plots.InteractivePlotimported but unused (F401)Remove unused import
150-150:
.core.plots.ViewPlotimported but unused (F401)Remove unused import
151-151:
.core.plots.ViewGridimported but unused (F401)Remove unused import
152-152:
.core.plots.CategoricalHistogramimported but unused (F401)Remove unused import
153-153:
.core.plots.NumericalHistogramimported but unused (F401)Remove unused import
155-159: Module level import not at top of file (E402)
156-156:
.core.runs.RunConfigimported but unused (F401)Remove unused import
157-157:
.core.runs.Runimported but unused (F401)Remove unused import
158-158:
.core.runs.RunResultsimported but unused (F401)Remove unused import
160-160: Module level import not at top of file (E402)
160-160:
.core.sample.Sampleimported but unused (F401)Remove unused import:
.core.sample.Sample
161-187: Module level import not at top of file (E402)
162-162:
.core.threed.BoxGeometryimported but unused (F401)Remove unused import
163-163:
.core.threed.CylinderGeometryimported but unused (F401)Remove unused import
164-164:
.core.threed.PlaneGeometryimported but unused (F401)Remove unused import
165-165:
.core.threed.SphereGeometryimported but unused (F401)Remove unused import
166-166:
.core.threed.FbxMeshimported but unused (F401)Remove unused import
167-167:
.core.threed.GltfMeshimported but unused (F401)Remove unused import
168-168:
.core.threed.ObjMeshimported but unused (F401)Remove unused import
169-169:
.core.threed.PlyMeshimported but unused (F401)Remove unused import
170-170:
.core.threed.StlMeshimported but unused (F401)Remove unused import
171-171:
.core.threed.PerspectiveCameraimported but unused (F401)Remove unused import
172-172:
.core.threed.PointLightimported but unused (F401)Remove unused import
173-173:
.core.threed.DirectionalLightimported but unused (F401)Remove unused import
174-174:
.core.threed.AmbientLightimported but unused (F401)Remove unused import
175-175:
.core.threed.SpotLightimported but unused (F401)Remove unused import
176-176:
.core.threed.PointCloudimported but unused (F401)Remove unused import
177-177:
.core.threed.MeshBasicMaterialimported but unused (F401)Remove unused import
178-178:
.core.threed.MeshDepthMaterialimported but unused (F401)Remove unused import
179-179:
.core.threed.MeshLambertMaterialimported but unused (F401)Remove unused import
180-180:
.core.threed.MeshPhongMaterialimported but unused (F401)Remove unused import
181-181:
.core.threed.PointCloudMaterialimported but unused (F401)Remove unused import
182-182:
.core.threed.Sceneimported but unused (F401)Remove unused import
183-183:
.core.threed.SceneBackgroundimported but unused (F401)Remove unused import
184-184:
.core.threed.Eulerimported but unused (F401)Remove unused import
185-185:
.core.threed.Quaternionimported but unused (F401)Remove unused import
186-186:
.core.threed.Vector3imported but unused (F401)Remove unused import
188-230: Module level import not at top of file (E402)
189-189:
.core.stages.Concatimported but unused (F401)Remove unused import
190-190:
.core.stages.Excludeimported but unused (F401)Remove unused import
191-191:
.core.stages.ExcludeByimported but unused (F401)Remove unused import
192-192:
.core.stages.ExcludeFieldsimported but unused (F401)Remove unused import
193-193:
.core.stages.ExcludeFramesimported but unused (F401)Remove unused import
194-194:
.core.stages.ExcludeGroupsimported but unused (F401)Remove unused import
195-195:
.core.stages.ExcludeLabelsimported but unused (F401)Remove unused import
196-196:
.core.stages.Existsimported but unused (F401)Remove unused import
197-197:
.core.stages.FilterFieldimported but unused (F401)Remove unused import
198-198:
.core.stages.FilterLabelsimported but unused (F401)Remove unused import
199-199:
.core.stages.FilterKeypointsimported but unused (F401)Remove unused import
200-200:
.core.stages.Flattenimported but unused (F401)Remove unused import
201-201:
.core.stages.GeoNearimported but unused (F401)Remove unused import
202-202:
.core.stages.GeoWithinimported but unused (F401)Remove unused import
203-203:
.core.stages.GroupByimported but unused (F401)Remove unused import
204-204:
.core.stages.Limitimported but unused (F401)Remove unused import
205-205:
.core.stages.LimitLabelsimported but unused (F401)Remove unused import
206-206:
.core.stages.MapLabelsimported but unused (F401)Remove unused import
207-207:
.core.stages.Matchimported but unused (F401)Remove unused import
208-208:
.core.stages.MatchFramesimported but unused (F401)Remove unused import
209-209:
.core.stages.MatchLabelsimported but unused (F401)Remove unused import
210-210:
.core.stages.MatchTagsimported but unused (F401)Remove unused import
211-211:
.core.stages.Mongoimported but unused (F401)Remove unused import
212-212:
.core.stages.Shuffleimported but unused (F401)Remove unused import
213-213:
.core.stages.Selectimported but unused (F401)Remove unused import
214-214:
.core.stages.SelectByimported but unused (F401)Remove unused import
215-215:
.core.stages.SelectFieldsimported but unused (F401)Remove unused import
216-216:
.core.stages.SelectFramesimported but unused (F401)Remove unused import
217-217:
.core.stages.SelectGroupsimported but unused (F401)Remove unused import
218-218:
.core.stages.SelectGroupSlicesimported but unused (F401)Remove unused import
219-219:
.core.stages.SelectLabelsimported but unused (F401)Remove unused import
220-220:
.core.stages.SetFieldimported but unused (F401)Remove unused import
221-221:
.core.stages.Skipimported but unused (F401)Remove unused import
222-222:
.core.stages.SortByimported but unused (F401)Remove unused import
223-223:
.core.stages.SortBySimilarityimported but unused (F401)Remove unused import
224-224:
.core.stages.Takeimported but unused (F401)Remove unused import
225-225:
.core.stages.ToPatchesimported but unused (F401)Remove unused import
226-226:
.core.stages.ToEvaluationPatchesimported but unused (F401)Remove unused import
227-227:
.core.stages.ToClipsimported but unused (F401)Remove unused import
228-228:
.core.stages.ToTrajectoriesimported but unused (F401)Remove unused import
229-229:
.core.stages.ToFramesimported but unused (F401)Remove unused import
231-235: Module level import not at top of file (E402)
232-232:
.core.session.close_appimported but unused (F401)Remove unused import
233-233:
.core.session.launch_appimported but unused (F401)Remove unused import
234-234:
.core.session.Sessionimported but unused (F401)Remove unused import
236-242: Module level import not at top of file (E402)
237-237:
.core.utils.disable_progress_barsimported but unused (F401)Remove unused import
238-238:
.core.utils.pprintimported but unused (F401)Remove unused import
239-239:
.core.utils.pformatimported but unused (F401)Remove unused import
240-240:
.core.utils.report_progressimported but unused (F401)Remove unused import
241-241:
.core.utils.ProgressBarimported but unused (F401)Remove unused import
243-243: Module level import not at top of file (E402)
243-243:
.core.view.DatasetViewimported but unused (F401)Remove unused import:
.core.view.DatasetView
244-248: Module level import not at top of file (E402)
245-245:
.utils.eval.classification.evaluate_classificationsimported but unused (F401)Remove unused import
246-246:
.utils.eval.classification.ClassificationEvaluationConfigimported but unused (F401)Remove unused import
247-247:
.utils.eval.classification.ClassificationResultsimported but unused (F401)Remove unused import
249-253: Module level import not at top of file (E402)
250-250:
.utils.eval.detection.evaluate_detectionsimported but unused (F401)Remove unused import
251-251:
.utils.eval.detection.DetectionEvaluationConfigimported but unused (F401)Remove unused import
252-252:
.utils.eval.detection.DetectionResultsimported but unused (F401)Remove unused import
254-258: Module level import not at top of file (E402)
255-255:
.utils.eval.regression.evaluate_regressionsimported but unused (F401)Remove unused import
256-256:
.utils.eval.regression.RegressionEvaluationConfigimported but unused (F401)Remove unused import
257-257:
.utils.eval.regression.RegressionResultsimported but unused (F401)Remove unused import
259-263: Module level import not at top of file (E402)
260-260:
.utils.eval.segmentation.evaluate_segmentationsimported but unused (F401)Remove unused import
261-261:
.utils.eval.segmentation.SegmentationEvaluationConfigimported but unused (F401)Remove unused import
262-262:
.utils.eval.segmentation.SegmentationResultsimported but unused (F401)Remove unused import
264-264: Module level import not at top of file (E402)
264-264:
.utils.quickstart.quickstartimported but unused (F401)Remove unused import:
.utils.quickstart.quickstarttests/unittests/label_tests.py
55-55: Use
key in dictinstead ofkey in dict.keys()(SIM118)Remove
.keys()
658-658: Local variable
id1is assigned to but never used (F841)Remove assignment to unused variable
id1
661-661: Local variable
id4is assigned to but never used (F841)Remove assignment to unused variable
id4fiftyone/core/labels.py
39-39: Class
_NoDefaultinherits fromobject(UP004)Remove
objectinheritance
100-100: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
126-128: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
127-127: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
268-268: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
307-310: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
308-309: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
315-318: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
316-317: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
343-343: Class
_HasLabelListinherits fromobject(UP004)Remove
objectinheritance
583-583: No explicit
stacklevelkeyword argument found (B028)
635-635: No explicit
stacklevelkeyword argument found (B028)
650-650: Class
_HasMediainherits fromobject(UP004)Remove
objectinheritance
1015-1018: Use ternary operator
_filled = filled if filled is not None else self.filledinstead ofif-else-block (SIM108)Replace
if-else-block with_filled = filled if filled is not None else self.filled
1892-1892: Avoid extraneous parentheses (UP034)
Remove extraneous parentheses
1897-1900: Use ternary operator
is_rgb = fof.is_rgb_target(target) if target is not None else Falseinstead ofif-else-block (SIM108)Replace
if-else-block withis_rgb = fof.is_rgb_target(target) if target is not None else False
1906-1909: Use ternary operator
dtype = int if target is not None and not is_rgb and target > 255 else np.uint8instead ofif-else-block (SIM108)Replace
if-else-block withdtype = int if target is not None and not is_rgb and target > 255 else np.uint8
1924-1927: Use ternary operator
is_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)instead ofif-else-block (SIM108)Replace
if-else-block withis_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)
2121-2121: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2135-2135: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2254-2255: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2264-2265: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
2274-2275: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
Additional comments not posted (5)
setup.py (1)
76-76: Addition oftifffileto handle TIFF files for the newDetectionsMaskclass.fiftyone/__public__.py (2)
96-96: The newDetectionsMaskclass is added to the public interface, ensuring it is accessible as part of the FiftyOne library's API.Tools
Ruff
96-96:
.core.labels.DetectionsMaskimported but unused (F401)Remove unused import
Line range hint
16-31: Many imports in this file are flagged as unused. This could either be a false positive due to dynamic usage not detected by static analysis, or these imports are genuinely unused. Verifying this would require a broader review of the usage within the entire codebase.Also applies to: 32-32, 33-33, 34-45, 46-50, 51-81, 82-82, 83-83, 84-107, 108-111, 112-116, 117-128, 129-138, 139-154, 155-159, 160-160, 161-187, 188-230, 231-235, 236-242, 243-243, 244-248, 249-253, 254-258, 259-263, 264-264
Tools
Ruff
93-93:
.core.labels.Classificationsimported but unused (F401)Remove unused import
94-94:
.core.labels.Detectionimported but unused (F401)Remove unused import
95-95:
.core.labels.Detectionsimported but unused (F401)Remove unused import
96-96:
.core.labels.DetectionsMaskimported but unused (F401)Remove unused import
97-97:
.core.labels.Polylineimported but unused (F401)Remove unused import
98-98:
.core.labels.Polylinesimported but unused (F401)Remove unused import
99-99:
.core.labels.Keypointimported but unused (F401)Remove unused import
tests/unittests/label_tests.py (1)
475-553: The unit tests for the newDetectionsMaskclass look comprehensive, covering various conversions and checks for consistency across different label types and transformations.fiftyone/core/labels.py (1)
650-650: Remove unnecessary inheritance fromobject.- class _HasMedia(object): + class _HasMedia:Likely invalid or redundant comment.
Tools
Ruff
650-650: Class
_HasMediainherits fromobject(UP004)Remove
objectinheritance
| if mask_targets is None: | ||
| is_rgb = False | ||
| else: | ||
| is_rgb = fof.is_rgb_mask_targets(mask_targets) |
There was a problem hiding this comment.
Use a ternary operator to simplify the assignment.
- if mask_targets is None:
- is_rgb = False
- else:
- is_rgb = fof.is_rgb_mask_targets(mask_targets)
+ is_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)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.
| if mask_targets is None: | |
| is_rgb = False | |
| else: | |
| is_rgb = fof.is_rgb_mask_targets(mask_targets) | |
| is_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets) |
Tools
Ruff
1924-1927: Use ternary operator
is_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)instead ofif-else-block (SIM108)Replace
if-else-block withis_rgb = False if mask_targets is None else fof.is_rgb_mask_targets(mask_targets)
|
@kemaleren thank you for the extremely thorough discussion of your thinking behind this PR! Very helpful 🙇 Just to establish some nomenclature in FiftyOne:
How would you evaluate data in your If 2, then, to clarify: one option that you didn't explicitly mention is just adding a factory method like so: detections = fo.Detections.from_mask(mask, mask_targets)where However, if I understand correctly, your problem is that when there are many objects, storing data in format 2 could generate > 16MB of data per sample? I expect the in-database size of a I find that thinking about new label types in terms of how they would be evaluated (ie what task they represent) to be a helpful exercise. In general, it would be ideal to have a 1-1 correspondence between Adding new |
|
@brimoor Sorry for not getting back to you earlier.
Like 2.
Yes, this factory method would be another way of implementing option #2 from my list of three options. If you prefer doing it this way, it's not too much work to adapt this PR. Let me know if that's the direction you want to go.
Exactly this, yes.
I know it's going to be a lot of work. I just don't see any other way around the 16MB limit, unless you were to switch to a different database, which would be even more work. Let me know what else I can help with. |
|
Closing, since this PR is superseded by #4829 |
What changes are proposed in this pull request?
This PR adds a new
Labelclass calledDetectionsMaskfor mask-based detections, analogous to how masks in theSegmentationwork for semantic segmentations. It also adds support for TIFF mask files, and it fixes an off-by-one bug in_parse_stuff_instance()and_parse_thing_instances().This functionality is needed because large images with lots of detections can easily exceed the 16mb storage capacity of a MongoDB document. Moreover, many computer vision models work directly with detection masks, so it makes sense for FiftyOne to support them. These masks can be easily compressed for efficient storage and bandwidth, and they can stored on disk just like segmentation masks. In the future, they could also be chunked for more efficient access, such as with Zarr.
These masks are similar to semantic segmentation masks, but instead of each value in the mask corresponding to a segmentation class, it corresponds to an individual object. For instance this mask for a 5x5 image:
with
labels = {1: “A”, 2: “A”, 3: “B”}corresponds to three detected objects, with labels “A” for the first two objects and “B” for the third.The new class can be used similarly to the
Segmentationsclass:I chose to implement this new
DetectionsMaskclass after considering multiple other options:Option 1: add a new mask type to
Segmentation, in addition to the'stuff'and'thing'types. This mask type would be called something like'objects'or'instances', to specify that each unique value in the mask corresponds to a single object, not to a connected component. I list this option for completeness, but I think it is the worst one, so it will not be considered further.'thing'segmentations, which is the same idea as this except that it uses connected components to separate objects. This option would simply adds the option to be explicit about masking individual objects, instead of connected components.'stuff'and'thing'distinction is already hard to remember. Adding a third would be even more confusing.Detectionsclass and sometimes useSegmentationclass, depending on how the data is stored.Option 2: Extend the
Detectionsclass to support amaskandmask_pathattribute, much like Segmentation does.Segmentation.Detectionsobject is being used to store a list ofDetectionobjects or a mask. Is themask, themask_path, or thedetectionslist the canonical set of detections? If it’sdetections, then what should thelabelsattribute be set to?LabeltoDetections, should it create a list of detections, or a mask?Option 3: Create a new class called
DetectionMask, which has amaskandmask_pathattribute likeSegmentationinstead of adetectionslist.Detectionslabel shouldn’t have to care about how the detections are stored.This PR implements option 3. If we decide option 2 make more sense, the two classes can be merged instead.
The main reason I’ve chosen option 3 is that
Detectionscan store overlapping detections, whereasDetectionsMaskcannot. It would be confusing if a single instance ofDetectionscould lose information depending on its internal representation. There is already precedence in FiftyOne for conversions between different label types to lose information.The tradeoff is now there are more changes that need to be made in the rest of the code base to support the new
DetectionsMaskclass. In most cases, I expect a simple conversion fromDetectionsMasktoDetections, which does not lose information, to be sufficient.Rather than duplicate the mask functionality from
Segmentation, the code for dealing with masks has been broken out into a_HasMaskmixin. It moves_read_mask()and_write_mask()into the mixin as abstract methods, so that each class using the mixin has to implement them. Making each class that uses the mixin implement these read/write functions keeps backwards compatibility for theSegmentationclass, which can store masks as either grayscale or RGB images. Storing masks as RGB images confuses the data (the mask itself) with its visual representation (the color), which does not make sense for object detections. Therefore,DetectionsMaskimplements these functions differently, to ensure the mask is always a grayscale PNG or TIFF. This approach of only allowing 2D arrays has the following advantages:Segmentationclass, casts masks to integers, which could lead to bugs or loss of data. It is better fail early if the user tries to save a floating-point mask that contains non-integer values.Other changes
_parse_stuff_instance()and_parse_thing_instance(). Reduced code duplication by reusing the former function as part of_parse_thing_instance(). Even if the rest of this PR is rejected, we should break this change out into its own PR.Segmentationtoo.Other issues
default_labeland a dictionarylabelsmapping each object index to a label. This method will still eventually run out of space in a 16mb document, if there are enough objects with different labels. See the notes below aboutdefault_label.Labeltypes.About
default_labellabels, and it gives a bit more information about theDetectionsMaskif it only contains one label, or one common label.SetField.Conversions to other
Labeltypes, and loss of informationLabeltypes store data, conversions between them often lose information. This was already the case before adding this newLabeltype, but because this PR adds new conversion options, I detail them here.DetectionMask<->Detectionsshould be identical (with possibly different representations) IF detections don’t overlapDetections->DetectionMasksloses information. Whichever overlapping detection gets rendered last wins.Detections/DetectionsMask<->Polylineloses information because of tolerance and overlap. When objects overlap, last rendered wins.Detections/DetectionsMask->Segmentationloses information because of overlapping or even connected objects being merged into a single segmentation class.Further work needed to complete this PR
DetectionsMaskclass with FiftyOne team. Decide if they want to add this functionality at all, and if so, whether it should be a separate class or merged withDetections.test_label_conversion()has gotten quite complicated. Maybe it should be broken up?DetectionsMasklabels, or decide to mergeDetectionsandDetectionsMask.Detectionobjects on the client side. It also probably should have the option to only loadDetectionMasklabels in the modal view, to avoid trying to show all the detections in the grid view.How is this patch tested? If it is not, please explain why.
Added tests to
tests/unittests/label_tests.py.Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Adds a new
Labelclass calledDetectionsMaskfor mask-based detections, with support for TIFF mask files. Fixes an off-by-one bug during mask conversion.What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
DetectionsMaskentity for enhanced mask-related operations.Enhancements
SegmentationandDetectionsMaskclasses.Dependencies
"tifffile"as a new dependency to support enhanced mask functionalities.