feat: ED-1658 Video Space + Image Space #1031
Conversation
Unit test report (Python 3.9.24, Pydantic 1.10.22)357 tests 331 ✅ 9s ⏱️ For more details on these failures, see this check. Results for commit 7c2743d. ♻️ This comment has been updated with latest results. |
Unit test report (Python 3.9.24, Pydantic 2.12.3)357 tests 331 ✅ 10s ⏱️ For more details on these failures, see this check. Results for commit 7c2743d. ♻️ This comment has been updated with latest results. |
Summary of ChangesHello @clinton-encord, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the concept of 'Spaces' to the Encord platform, allowing for better organization and management of annotations within video and image data. It includes the implementation of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature, "Spaces", which allows for more complex data organization like video and image groups. The implementation is extensive, adding new classes for Space, VideoSpace, ImageSpace, and various Annotation types. The use of TypedDict for better type safety is a great improvement.
However, there are several areas that need attention:
- There is some code duplication that should be refactored.
- Some parts of the implementation seem incomplete, especially around parsing labels for
ImageSpace. - There are potential bugs in how object instances are shared and filtered across different spaces.
- Some leftover debug code needs to be cleaned up.
Overall, this is a good step forward, but the identified issues should be addressed to ensure correctness and maintainability.
| GeometricCoordinates = Union[ | ||
| BoundingBoxCoordinates, | ||
| RotatableBoundingBoxCoordinates, | ||
| PointCoordinate, | ||
| PolygonCoordinates, | ||
| PolylineCoordinates, | ||
| SkeletonCoordinates, | ||
| BitmaskCoordinates, | ||
| ] |
There was a problem hiding this comment.
The GeometricCoordinates union type is missing PointCoordinate3D. This causes a type inconsistency in get_two_dimensional_coordinates_from_frame_object_label, which is typed to return GeometricCoordinates but can return a PointCoordinate3D instance. Please add PointCoordinate3D to the GeometricCoordinates union to fix this.
| GeometricCoordinates = Union[ | |
| BoundingBoxCoordinates, | |
| RotatableBoundingBoxCoordinates, | |
| PointCoordinate, | |
| PolygonCoordinates, | |
| PolylineCoordinates, | |
| SkeletonCoordinates, | |
| BitmaskCoordinates, | |
| ] | |
| GeometricCoordinates = Union[ | |
| BoundingBoxCoordinates, | |
| RotatableBoundingBoxCoordinates, | |
| PointCoordinate, | |
| PointCoordinate3D, | |
| PolygonCoordinates, | |
| PolylineCoordinates, | |
| SkeletonCoordinates, | |
| BitmaskCoordinates, | |
| ] |
| # Objects in space | ||
| # TODO: FIX THIS! How to properly get all object_instances | ||
| for space in self._space_map.values(): | ||
| for object_ in space._objects_map.values(): | ||
| # filter by ontology object | ||
| if not ( | ||
| filter_ontology_object is None | ||
| or object_.ontology_item.feature_node_hash == filter_ontology_object.feature_node_hash | ||
| ): | ||
| continue | ||
|
|
||
| # filter by frame | ||
| if filter_frames is None: | ||
| append = True | ||
| else: | ||
| append = False | ||
| for frame in filtered_frames_list: | ||
| hashes = self._frame_to_hashes.get(frame, set()) | ||
| if object_.object_hash in hashes: | ||
| append = True | ||
| break | ||
|
|
||
| if append: | ||
| ret.append(object_) | ||
|
|
There was a problem hiding this comment.
The logic for filtering objects in spaces by frame seems incorrect. It uses self._frame_to_hashes, which is populated for objects on the main label row, not for objects within spaces. This will likely result in objects on spaces not being correctly filtered by frame. The TODO comment also indicates this is an issue. You should use a mechanism internal to the space to check if an object is on a given frame.
| def _parse_space_labels( | ||
| self, | ||
| spaces_info: dict[str, SpaceInfo], | ||
| object_answers: dict, | ||
| classification_answers: dict, | ||
| ) -> None: | ||
| res: dict[str, Space] = dict() | ||
|
|
||
| for space_id, space_info in spaces_info.items(): | ||
| if space_info["space_type"] == SpaceType.VIDEO: | ||
| video_space = self.get_space_by_id(space_id=space_id, type_=VideoSpace) | ||
| video_space._parse_space_dict( | ||
| space_info, object_answers=object_answers, classification_answers=classification_answers | ||
| ) | ||
| res[space_id] = video_space | ||
| elif space_info["space_type"] == SpaceType.IMAGE: | ||
| image_space = ImageSpace( | ||
| space_id=space_id, | ||
| title=space_info["title"], | ||
| parent=self, | ||
| width=space_info["width"], | ||
| height=space_info["height"], | ||
| ) | ||
| # image_space._parse_space_dict( | ||
| # space_info, object_answers=object_answers, classification_answers=classification_answers | ||
| # ) | ||
| res[space_id] = image_space | ||
|
|
There was a problem hiding this comment.
In _parse_space_labels, the logic for ImageSpace seems incomplete as the call to image_space._parse_space_dict is commented out. This will lead to labels on image spaces not being parsed. Also, a new ImageSpace is instantiated here, but it should be retrieved using get_space_by_id as the spaces are already initialized. Finally, the res dictionary is initialized but never used and can be removed.
def _parse_space_labels(
self,
spaces_info: dict[str, SpaceInfo],
object_answers: dict,
classification_answers: dict,
) -> None:
for space_id, space_info in spaces_info.items():
if space_info["space_type"] == SpaceType.VIDEO:
video_space = self.get_space_by_id(space_id=space_id, type_=VideoSpace)
video_space._parse_space_dict(
space_info, object_answers=object_answers, classification_answers=classification_answers
)
elif space_info["space_type"] == SpaceType.IMAGE:
image_space = self.get_space_by_id(space_id=space_id, type_=ImageSpace)
image_space._parse_space_dict(
space_info, object_answers=object_answers, classification_answers=classification_answers
)
encord/objects/spaces/image_space.py
Outdated
| def _parse_frame_label_dict(self, frame_label: LabelBlob, classification_answers: dict): | ||
| for frame_object_label in frame_label["objects"]: | ||
| object_hash = frame_object_label["objectHash"] | ||
| object = self.parent._space_objects_map.get(object_hash) |
There was a problem hiding this comment.
In _parse_frame_label_dict, you are trying to find an existing object instance from self.parent._space_objects_map. However, this map is never populated in LabelRowV2. A similar logic in video_space.py iterates through all spaces to find the object. This inconsistency should be resolved, and a proper mechanism to share object instances across spaces should be implemented, likely by populating _space_objects_map in LabelRowV2.
encord/objects/coordinates.py
Outdated
| def get_two_dimensional_coordinates_from_frame_object_label( | ||
| frame_object_label: FrameObject, | ||
| ) -> GeometricCoordinates: | ||
| if frame_object_label["shape"] == Shape.BOUNDING_BOX: | ||
| return BoundingBoxCoordinates.from_dict(frame_object_label) | ||
| elif frame_object_label["shape"] == Shape.ROTATABLE_BOUNDING_BOX: | ||
| return RotatableBoundingBoxCoordinates.from_dict(frame_object_label) | ||
| elif frame_object_label["shape"] == Shape.POLYGON: | ||
| return PolygonCoordinates.from_dict(frame_object_label) | ||
| elif frame_object_label["shape"] == Shape.POINT: | ||
| coords = frame_object_label["point"]["0"] | ||
| if "x" in coords and "y" in coords and "z" in coords: | ||
| return PointCoordinate3D.from_dict(frame_object_label) # type: ignore | ||
| elif "x" in coords and "y" in coords: | ||
| return PointCoordinate.from_dict(frame_object_label) # type: ignore | ||
| else: | ||
| raise ValueError(f"Invalid point coordinates in {frame_object_label}") | ||
| elif frame_object_label["shape"] == Shape.POLYLINE: | ||
| return PolylineCoordinates.from_dict(frame_object_label) | ||
| elif "skeleton" in frame_object_label: | ||
|
|
||
| def _with_visibility_enum(point: dict): | ||
| if point.get(Visibility.INVISIBLE.value): | ||
| point["visibility"] = Visibility.INVISIBLE | ||
| elif point.get(Visibility.OCCLUDED.value): | ||
| point["visibility"] = Visibility.OCCLUDED | ||
| elif point.get(Visibility.SELF_OCCLUDED.value): | ||
| point["visibility"] = Visibility.SELF_OCCLUDED | ||
| elif point.get(Visibility.VISIBLE.value): | ||
| point["visibility"] = Visibility.VISIBLE | ||
| return point | ||
|
|
||
| values = [_with_visibility_enum(pnt) for pnt in frame_object_label["skeleton"].values()] | ||
| skeleton_frame_object_label = { | ||
| "name": frame_object_label["name"], | ||
| "values": values, | ||
| } | ||
| return SkeletonCoordinates.from_dict(skeleton_frame_object_label) | ||
| elif "bitmask" in frame_object_label: | ||
| return BitmaskCoordinates.from_dict(frame_object_label) | ||
| elif "cuboid" in frame_object_label: | ||
| raise NotImplementedError("Cuboid is not a two dimensional coordinate.") | ||
| else: | ||
| raise NotImplementedError(f"Getting coordinates for `{frame_object_label}` is not supported yet.") |
There was a problem hiding this comment.
The function get_two_dimensional_coordinates_from_frame_object_label duplicates most of its logic from get_coordinates_from_frame_object_label. This can be refactored to improve maintainability by calling get_coordinates_from_frame_object_label and then handling the cases specific to two-dimensional coordinates.
For example:
def get_two_dimensional_coordinates_from_frame_object_label(
frame_object_label: FrameObject,
) -> GeometricCoordinates:
coordinates = get_coordinates_from_frame_object_label(frame_object_label)
if isinstance(coordinates, CuboidCoordinates):
raise NotImplementedError("Cuboid is not a two dimensional coordinate.")
# This would require ensuring GeometricCoordinates includes all possible return types.
return coordinatesAdditionally, the nested helper function _with_visibility_enum is duplicated in both functions and could be extracted to a module-level helper to be reused.
encord/objects/types.py
Outdated
| class MyDict(TypedDict): | ||
| something: str | ||
|
|
||
|
|
||
| def hello(my_dict: MyDict) -> None: | ||
| yo = my_dict.get("something") | ||
| print(yo) |
There was a problem hiding this comment.
The MyDict class and hello function appear to be leftover debugging or testing code. They should be removed before merging.
SDK integration test report285 tests ±0 241 ✅ - 36 14m 28s ⏱️ - 3m 44s For more details on these failures, see this check. Results for commit 7c2743d. ± Comparison against base commit dbabc87. ♻️ This comment has been updated with latest results. |
clinton-encord
force-pushed
the
clinton/ed-1658/video-image-space
branch
from
f82d178 to
da48601
Compare
clinton-encord
force-pushed
the
clinton/ed-1658/video-image-space
branch
from
2aeeb05 to
7c2743d
Compare
Introduction and Explanation
Implement VideoSpace and ImageSpace.
JIRA
Link ticket(s)
Documentation
There should be enough internal documentation for a product owner to write customer-facing documentation or a separate PR linked if writing the customer documentation directly. Link all that are relevant below.
Tests
Make a quick statement and post any relevant links of CI / test results. If the testing infrastructure isn’t yet in-place, note that instead.
Known issues
If there are any known issues with the solution, make a statement about what they are and why they are Ok to leave unsolved for now. Make tickets for the known issues linked to the original ticket linked above