Support non-sequential COCO category IDs #4354
Conversation
…f classes or a dict mapping ids to labels
fix: coco category ids can now be not sequential - avoiding memory leak
WalkthroughThe recent updates in the FiftyOne package primarily focus on enhancing the handling of class labels within the COCO dataset functionalities. The changes introduce a more flexible Changes
Assessment against linked issues
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
Out of diff range and nitpick comments (4)
fiftyone/zoo/datasets/__init__.py (2)
Line range hint
120-120: Avoid using bareexceptstatements; specify exception types to handle expected errors more gracefully.- except: + except OSError:
Line range hint
577-577: Avoid using bareexceptstatements; specify exception types to handle expected errors more gracefully.- except: + except OSError:fiftyone/utils/coco.py (2)
417-418: Initialization of_classes_mapand_class_idsshould be documented in the class docstring to clarify their usage and importance.Consider adding a brief explanation in the class docstring about the purpose of
_classes_mapand_class_ids.
Line range hint
605-605: Ambiguous variable namelused in list comprehension. Consider using a more descriptive name.- license_map = {l.get("id", None): l.get(self.include_license, None) for l in info.get("licenses", [])} + license_map = {license.get("id", None): license.get(self.include_license, None) for license in info.get("licenses", [])}
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- fiftyone/utils/coco.py (40 hunks)
- fiftyone/zoo/datasets/init.py (1 hunks)
Additional Context Used
Ruff (12)
fiftyone/utils/coco.py (10)
605-605: Ambiguous variable name:
l
908-908: Do not use bare
except
918-918: Ambiguous variable name:
l
1799-1799: Ambiguous variable name:
l
1824-1824: Avoid equality comparisons to
True; useif include_license:for truth checks
1988-1988: Ambiguous variable name:
l
2042-2042: Do not use bare
except
2139-2139: Avoid equality comparisons to
True; useif extra_attrs:for truth checks
2142-2142: Avoid equality comparisons to
False; useif not extra_attrs:for false checks
2264-2264: Do not use bare
exceptfiftyone/zoo/datasets/__init__.py (2)
120-120: Do not use bare
except
577-577: Do not use bare
except
Additional comments not posted (1)
fiftyone/zoo/datasets/__init__.py (1)
366-366: Ensure thatinfo.classesis notNonebefore assigning todataset.default_classes.Verification successful
The review comment suggested ensuring that
info.classesis notNonebefore assigning it todataset.default_classes. The script output confirms that there is a check forinfo.classesnot beingNonebefore the assignment occurs:if info.classes is not None and not dataset.default_classes: dataset.default_classes = info.classesAdditionally, there is another usage where
info.classesis set based on another variableclassesifinfo.classesis not already set:if classes and not info.classes: info.classes = classesBoth usages in the code handle the
info.classesattribute carefully, ensuring it is notNonebefore it is used in a critical assignment or set based on another variable. This aligns with the review comment's concern, and the implementation adheres to the suggested precaution.Conclusion: The code correctly checks that
info.classesis notNonebefore using it in assignments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `info.classes` is not `None` before assignment. # Test: Search for the usage of `info.classes` in the file. Expect: Proper null checks before usage. rg --type python $'info.classes' fiftyone/zoo/datasets/__init__.pyLength of output: 96
Script:
#!/bin/bash # Description: Verify that `info.classes` is not `None` before assignment. # Correcting the previous script error by removing the unrecognized file type flag. rg 'info.classes' fiftyone/zoo/datasets/__init__.pyLength of output: 255
| classes_map, | ||
| supercategory_map, | ||
| images, | ||
| annotations, | ||
| ) = load_coco_detection_annotations( | ||
| self.labels_path, extra_attrs=self.extra_attrs | ||
| ) | ||
|
|
||
| classes = None | ||
| if classes_map is not None: | ||
| classes = _to_classes(classes_map) | ||
|
|
There was a problem hiding this comment.
The variable classes is redefined which might lead to confusion. Consider renaming it to avoid shadowing the outer scope variable.
- classes = None
- if classes_map is not None:
- classes = _to_classes(classes_map)
+ local_classes = None
+ if classes_map is not None:
+ local_classes = _to_classes(classes_map)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.
| classes_map, | |
| supercategory_map, | |
| images, | |
| annotations, | |
| ) = load_coco_detection_annotations( | |
| self.labels_path, extra_attrs=self.extra_attrs | |
| ) | |
| classes = None | |
| if classes_map is not None: | |
| classes = _to_classes(classes_map) | |
| classes_map, | |
| supercategory_map, | |
| images, | |
| annotations, | |
| ) = load_coco_detection_annotations( | |
| self.labels_path, extra_attrs=self.extra_attrs | |
| ) | |
| local_classes = None | |
| if classes_map is not None: | |
| local_classes = _to_classes(classes_map) | |
fiftyone/utils/coco.py
Outdated
| try: | ||
| categories = _info.get("categories", None) | ||
| parse_coco_categories(categories) | ||
| except: |
There was a problem hiding this comment.
Avoid using a bare except statement. Specify the exception type to handle specific exceptions only.
- except:
+ except Exception as e:
+ logger.error("Failed to parse categories: %s", str(e))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.
| except: | |
| except Exception as e: | |
| logger.error("Failed to parse categories: %s", str(e)) |
| "name": l, | ||
| "supercategory": None, | ||
| } | ||
| for i, l in enumerate(classes) |
There was a problem hiding this comment.
Ambiguous variable name l used in list comprehension. Consider using a more descriptive name.
- categories = [{ "id": i, "name": l, "supercategory": None } for i, l in enumerate(classes)]
+ categories = [{ "id": i, "name": label, "supercategory": None } for i, label in enumerate(classes)]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.
| for i, l in enumerate(classes) | |
| for i, label in enumerate(classes) |
fiftyone/utils/coco.py
Outdated
|
|
||
| try: | ||
| classes_map, _ = parse_coco_categories(categories) | ||
| except: |
There was a problem hiding this comment.
Avoid using a bare except statement. Specify the exception type to handle specific exceptions only.
- except:
+ except Exception as e:
+ logger.error("Failed to parse categories from info: %s", str(e))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.
| except: | |
| except Exception as e: | |
| logger.error("Failed to parse categories from info: %s", str(e)) |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (13)
fiftyone/utils/coco.py (7)
Line range hint
605-605: Ambiguous variable namelused in list comprehension. Consider using a more descriptive name.- license_map = {l.get("id", None): l.get(self.include_license, None) for l in info.get("licenses", [])} + license_map = {license_entry.get("id", None): license_entry.get(self.include_license, None) for license_entry in info.get("licenses", [])}
Line range hint
1795-1795: Ambiguous variable namelused in list comprehension. Consider using a more descriptive name.- categories = [{ "id": i, "name": l, "supercategory": None } for i, l in enumerate(classes)] + categories = [{ "id": i, "name": label, "supercategory": None } for i, label in enumerate(classes)]
Line range hint
1984-1984: Ambiguous variable namelused in list comprehension. Consider using a more descriptive name.- categories = [{ "id": i, "name": l, "supercategory": None } for i, l in enumerate(classes)] + categories = [{ "id": i, "name": label, "supercategory": None } for i, label in enumerate(classes)]
Line range hint
1820-1820: Avoid equality comparisons toTrue; useif include_license:for truth checks.- if include_license == True: + if include_license:
Line range hint
2127-2127: Avoid equality comparisons toTrue; useif extra_attrs:for truth checks.- if extra_attrs == True: + if extra_attrs:
Line range hint
2130-2130: Avoid equality comparisons toFalse; useif not extra_attrs:for false checks.- if extra_attrs == False: + if not extra_attrs:
Line range hint
2252-2252: Do not use bareexcept.- except: + except Exception:tests/unittests/import_export_tests.py (6)
Line range hint
2160-2160: Remove unused import ofpydicom.- import pydicom # pylint: disable=unused-import
Line range hint
3598-3598: Remove unused local variableimg_filepath.- img_filepath = self._new_image(name="openlabel_test")
Line range hint
3615-3615: Remove unused local variableimg_filepath.- img_filepath = self._new_image(name="openlabel_test")
Line range hint
3633-3633: Remove unused local variableimg_filepath.- img_filepath = self._new_image(name="openlabel_test")
Line range hint
3662-3662: Remove unused local variableimg_filepath.- img_filepath = self._new_image(name="openlabel_test")
Line range hint
3732-3732: Remove unused local variablevid_filepath.- vid_filepath = self._new_video(filename="openlabel_test")
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- docs/source/user_guide/export_datasets.rst (1 hunks)
- fiftyone/utils/coco.py (42 hunks)
- tests/unittests/import_export_tests.py (1 hunks)
Additional Context Used
Ruff (14)
fiftyone/utils/coco.py (8)
605-605: Ambiguous variable name:
l
917-917: Ambiguous variable name:
l
1795-1795: Ambiguous variable name:
l
1820-1820: Avoid equality comparisons to
True; useif include_license:for truth checks
1984-1984: Ambiguous variable name:
l
2127-2127: Avoid equality comparisons to
True; useif extra_attrs:for truth checks
2130-2130: Avoid equality comparisons to
False; useif not extra_attrs:for false checks
2252-2252: Do not use bare
excepttests/unittests/import_export_tests.py (6)
2160-2160:
pydicomimported but unused
3598-3598: Local variable
img_filepathis assigned to but never used
3615-3615: Local variable
img_filepathis assigned to but never used
3633-3633: Local variable
img_filepathis assigned to but never used
3662-3662: Local variable
img_filepathis assigned to but never used
3732-3732: Local variable
vid_filepathis assigned to but never used
Additional comments not posted (5)
fiftyone/utils/coco.py (5)
132-133: Update the docstring to reflect the change fromclassestoclasses_map.
198-201: Ensure that the conversion from list to dictionary uses 1-based indexing ifclassesis a list, to maintain consistency with COCO's 1-based ID system.
460-462: The conditional check foronly_matchingandself._class_idsshould be simplified to improve readability.
558-569: The variableclassesis redefined which might lead to confusion. Consider renaming it to avoid shadowing the outer scope variable.
917-917: Ambiguous variable namelused in list comprehension. Consider using a more descriptive name.
Resolves #4293, Resolves #4162
Extends #4309 to support the full "spec" described in #4293 (comment).
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements