- Use HtmlCoordinates and TextCoordinates to hold location of labels …

…on text files

- Only allow classifications on frame=0 for non-geometric files

encord/objects/classification_instance.py

@@ -31,7 +31,7 @@
 
 from encord.common.range_manager import RangeManager
 from encord.common.time_parser import parse_datetime
-from encord.constants.enums import DataType
+from encord.constants.enums import DataType, is_geometric
 from encord.exceptions import LabelRowError
 from encord.objects.answers import Answer, ValueType, _get_static_answer_map
 from encord.objects.attributes import (
@@ -55,6 +55,17 @@
     from encord.objects import LabelRowV2
 
 
+# For Audio and Text files, classifications can only be applied to Range(start=0, end=0)
+# Because we treat the entire file as being on one frame (for classifications, its different for objects)
+def _verify_non_geometric_classifications_range(ranges_to_add: Ranges, label_row: Optional[LabelRowV2]) -> None:
+    is_range_only_on_frame_0 = len(ranges_to_add) == 1 and ranges_to_add[0].start == 0 and ranges_to_add[0].end == 0
+    if label_row is not None and not is_geometric(label_row.data_type) and not is_range_only_on_frame_0:
+        raise LabelRowError(
+            "For audio files and text files, classifications can only be attached to frame=0 "
+            "You may use `ClassificationInstance.set_for_frames(frames=Range(start=0, end=0))`."
+        )
+
+
 class ClassificationInstance:
     def __init__(
         self,
@@ -104,6 +115,10 @@ def feature_hash(self) -> str:
     def _last_frame(self) -> Union[int, float]:
         if self._parent is None or self._parent.data_type is DataType.DICOM:
             return float("inf")
+        elif self._parent is not None and self._parent.data_type == "text/html":
+            # For HTML files, the entire file is treated as one frame
+            # Note: for Audio and Plain Text, classifications must be applied to ALL the "frames"
+            return 1
         else:
             return self._parent.number_of_frames
 
@@ -139,22 +154,25 @@ def _set_for_ranges(
         reviews: Optional[List[dict]],
     ):
         new_range_manager = RangeManager(frame_class=frames)
-        conflicting_ranges = self._is_classification_already_present_on_range(new_range_manager.get_ranges())
+        ranges_to_add = new_range_manager.get_ranges()
+
+        _verify_non_geometric_classifications_range(ranges_to_add, self._parent)
+
+        conflicting_ranges = self._is_classification_already_present_on_range(ranges_to_add)
         if conflicting_ranges and not overwrite:
             raise LabelRowError(
                 f"The classification '{self.classification_hash}' already exists "
                 f"on the ranges {conflicting_ranges}. "
                 f"Set 'overwrite' parameter to True to override."
             )
 
-        ranges_to_add = new_range_manager.get_ranges()
         for range_to_add in ranges_to_add:
             self._check_within_range(range_to_add.end)
 
         """
         At this point, this classification instance operates on ranges, NOT on frames.
-        We therefore leave only FRAME 0 in the map.The frame_data for FRAME 0 will be
-        treated as the data for all "frames" in this classification instance.
+        We therefore leave only FRAME 0 in the map. The frame_data for FRAME 0 will be
+        treated as the data for the entire classification instance.
         """
         self._set_frame_and_frame_data(
             frame=0,
@@ -221,8 +239,6 @@ def set_for_frames(
             last_edited_at = datetime.now()
 
         if self._range_only:
-            # Audio range should cover entire audio file
-            # Text range should always be [0, 0]
             self._set_for_ranges(
                 frames=frames,
                 overwrite=overwrite,
@@ -687,7 +703,9 @@ def _is_selectable_child_attribute(self, attribute: Attribute) -> bool:
     def _check_within_range(self, frame: int) -> None:
         if frame < 0 or frame >= self._last_frame:
             raise LabelRowError(
-                f"The supplied frame of `{frame}` is not within the acceptable bounds of `0` to `{self._last_frame}`."
+                f"The supplied frame of `{frame}` is not within the acceptable bounds of `0` to `{self._last_frame}`. "
+                f"Note: for non-geometric data (e.g. {DataType.AUDIO} and {DataType.PLAIN_TEXT}), "
+                f"the entire file has only 1 frame."
             )
 
     def _is_classification_already_present(self, frames: Iterable[int]) -> Set[int]:

encord/objects/coordinates.py

@@ -359,30 +359,31 @@ class TextCoordinates(BaseDTO):
     Represents coordinates for a text file
 
     Attributes:
-        range_html (List[HtmlRange]): A list of HtmlRange objects
         range (Ranges): Ranges of chars for simple text files
     """
 
-    range_html: Optional[List[HtmlRange]] = None
-    range: Optional[Ranges] = None
+    range: Ranges
 
-    def __post_init__(self):
-        if self.range_html is None and self.range is None:
-            raise ValueError("At least one of either `range` or `range_html` must be set.")
 
-        if self.range_html is not None and self.range is not None:
-            raise ValueError("Only one of either `range` or `range_html` must be set.")
+class HtmlCoordinates(BaseDTO):
+    """
+    Represents coordinates for a html file
+
+    Attributes:
+        range_html (List[HtmlRange]): A list of HtmlRange objects
+    """
 
-        if self.range_html is not None and len(self.range_html) == 0:
-            raise ValueError("Range HTML list must contain at least one html range.")
+    range: List[HtmlRange]
 
-        if self.range is not None and len(self.range) == 0:
-            raise ValueError("Range list must contain at least one range.")
+
+NON_GEOMETRIC_COORDINATES = {AudioCoordinates, TextCoordinates, HtmlCoordinates}
 
 
 Coordinates = Union[
     AudioCoordinates,
     TextCoordinates,
+    Union[HtmlCoordinates, TextCoordinates],
+    HtmlCoordinates,
     BoundingBoxCoordinates,
     RotatableBoundingBoxCoordinates,
     PointCoordinate,
@@ -392,14 +393,14 @@ def __post_init__(self):
     BitmaskCoordinates,
 ]
 
-ACCEPTABLE_COORDINATES_FOR_ONTOLOGY_ITEMS: Dict[Shape, Type[Coordinates]] = {
-    Shape.BOUNDING_BOX: BoundingBoxCoordinates,
-    Shape.ROTATABLE_BOUNDING_BOX: RotatableBoundingBoxCoordinates,
-    Shape.POINT: PointCoordinate,
-    Shape.POLYGON: PolygonCoordinates,
-    Shape.POLYLINE: PolylineCoordinates,
-    Shape.SKELETON: SkeletonCoordinates,
-    Shape.BITMASK: BitmaskCoordinates,
-    Shape.AUDIO: AudioCoordinates,
-    Shape.TEXT: TextCoordinates,
+ACCEPTABLE_COORDINATES_FOR_ONTOLOGY_ITEMS: Dict[Shape, List[Type[Coordinates]]] = {
+    Shape.BOUNDING_BOX: [BoundingBoxCoordinates],
+    Shape.ROTATABLE_BOUNDING_BOX: [RotatableBoundingBoxCoordinates],
+    Shape.POINT: [PointCoordinate],
+    Shape.POLYGON: [PolygonCoordinates],
+    Shape.POLYLINE: [PolylineCoordinates],
+    Shape.SKELETON: [SkeletonCoordinates],
+    Shape.BITMASK: [BitmaskCoordinates],
+    Shape.AUDIO: [AudioCoordinates],
+    Shape.TEXT: [TextCoordinates, HtmlCoordinates],
 }

encord/objects/html_node.py

@@ -56,7 +56,7 @@ def to_dict(self):
         }
 
     def __hash__(self):
-        return hash(self.__repr__())
+        return f"{self.start.node}-{self.start.offset}-{self.end.node}-{self.end.offset}"
 
     @classmethod
     def from_dict(cls, d: dict):

encord/objects/ontology_labels_impl.py

@@ -40,7 +40,7 @@
     BundledWorkflowReopenPayload,
 )
 from encord.objects.classification import Classification
-from encord.objects.classification_instance import ClassificationInstance
+from encord.objects.classification_instance import ClassificationInstance, _verify_non_geometric_classifications_range
 from encord.objects.constants import (  # pylint: disable=unused-import # for backward compatibility
     DATETIME_LONG_STRING_FORMAT,
     DEFAULT_CONFIDENCE,
@@ -51,6 +51,7 @@
     BitmaskCoordinates,
     BoundingBoxCoordinates,
     Coordinates,
+    HtmlCoordinates,
     PointCoordinate,
     PolygonCoordinates,
     PolylineCoordinates,
@@ -833,9 +834,15 @@ def add_object_instance(self, object_instance: ObjectInstance, force: bool = Tru
         # We want to ensure that we are only adding the object_instance to a label_row
         # IF AND ONLY IF the file type is text/html and the object_instance has range_html set
         if self.file_type == "text/html" and object_instance.range_html is None:
-            raise LabelRowError("Unable to assign object instance without a html range to a html file")
+            raise LabelRowError(
+                "Unable to assign object instance without a html range to a html file. "
+                f"Please ensure the object instance exists on frame=0, and has coordinates of type {HtmlCoordinates}."
+            )
         elif self.file_type != "text/html" and object_instance.range_html is not None:
-            raise LabelRowError("Unable to assign object instance with a html range to a non-html file")
+            raise LabelRowError(
+                "Unable to assign object instance with a html range to a non-html file. "
+                f"Please ensure the object instance does not have coordinates of type {HtmlCoordinates}."
+            )
 
         if object_instance.is_assigned_to_label_row():
             raise LabelRowError(
@@ -898,10 +905,10 @@ def add_classification_instance(self, classification_instance: ClassificationIns
             )
 
         """
-        Implementation here diverges because audio data will operate on ranges, whereas
+        Implementation here diverges because non-geometric data will operate on ranges, whereas
         everything else will operate on frames.
         """
-        if self.data_type == DataType.AUDIO:
+        if not is_geometric(self.data_type):
             self._add_classification_instance_for_range(
                 classification_instance=classification_instance,
                 force=force,
@@ -930,14 +937,18 @@ def add_classification_instance(self, classification_instance: ClassificationIns
             self._classifications_to_frames[classification_instance.ontology_item].update(frames)
             self._add_to_frame_to_hashes_map(classification_instance, frames)
 
-    # This should only be used for Audio classification instances
+    # This should only be used for Non-geometric data
     def _add_classification_instance_for_range(
         self,
         classification_instance: ClassificationInstance,
         force: bool,
     ):
         classification_hash = classification_instance.classification_hash
         ranges_to_add = classification_instance.range_list
+
+        if classification_instance.is_range_only():
+            _verify_non_geometric_classifications_range(ranges_to_add, self)
+
         already_present_ranges = self._is_classification_already_present_on_ranges(
             classification_instance.ontology_item, ranges_to_add
         )
@@ -970,7 +981,7 @@ def remove_classification(self, classification_instance: ClassificationInstance)
         classification_hash = classification_instance.classification_hash
         self._classifications_map.pop(classification_hash)
 
-        if self.data_type == DataType.AUDIO:
+        if not is_geometric(self.data_type):
             range_manager = self._classifications_to_ranges[classification_instance.ontology_item]
             ranges_to_remove = classification_instance.range_list
             range_manager.remove_ranges(ranges_to_remove)
@@ -2326,7 +2337,7 @@ def _create_new_html_object_instance(
 
         object_instance = ObjectInstance(label_class, object_hash=object_hash)
         object_instance.set_for_frames(
-            TextCoordinates(range_html=[HtmlRange.from_dict(x) for x in range_html]),
+            HtmlCoordinates(range=[HtmlRange.from_dict(x) for x in range_html]),
             frames=0,
             created_at=object_frame_instance_info.created_at,
             created_by=object_frame_instance_info.created_by,

encord/objects/ontology_object_instance.py

@@ -43,8 +43,10 @@
 from encord.objects.constants import DEFAULT_CONFIDENCE, DEFAULT_MANUAL_ANNOTATION
 from encord.objects.coordinates import (
     ACCEPTABLE_COORDINATES_FOR_ONTOLOGY_ITEMS,
+    NON_GEOMETRIC_COORDINATES,
     AudioCoordinates,
     Coordinates,
+    HtmlCoordinates,
     TextCoordinates,
 )
 from encord.objects.frames import (
@@ -177,8 +179,8 @@ def range_html(self) -> Optional[HtmlRanges]:
 
         coordinates = non_geometric_annotation.coordinates
 
-        if isinstance(coordinates, TextCoordinates):
-            return coordinates.range_html
+        if isinstance(coordinates, HtmlCoordinates):
+            return coordinates.range
         else:
             return None
 
@@ -483,7 +485,7 @@ def set_for_frames(
         """
 
         if self._non_geometric:
-            if not isinstance(coordinates, AudioCoordinates) and not isinstance(coordinates, TextCoordinates):
+            if not isinstance(coordinates, tuple(NON_GEOMETRIC_COORDINATES)):
                 raise LabelRowError("Expecting non-geometric coordinate type")
 
             elif frames != 0:
@@ -502,24 +504,11 @@ def set_for_frames(
                     "Cannot overwrite existing data for a frame. Set `overwrite` to `True` to overwrite."
                 )
 
-            check_coordinate_type(coordinates, self._ontology_object)
+            check_coordinate_type(coordinates, self._ontology_object, self._parent)
 
-            if isinstance(coordinates, (AudioCoordinates, TextCoordinates)) and coordinates.range is not None:
-                if self._parent is not None and self._parent == "text/html":
-                    raise LabelRowError(
-                        "For html labels, ensure the `range_html` property "
-                        "is set when instantiating the TextCoordinates."
-                    )
-
-                self.check_within_range(coordinates.range[0].end)
-
-            elif isinstance(coordinates, TextCoordinates) and coordinates.range_html is not None:
-                if self._parent is not None and self._parent.file_type != "text/html":
-                    raise LabelRowError(
-                        "For non-html labels, ensure the `range` property "
-                        "is set when instantiating the TextCoordinates."
-                    )
-                # Unable to validate xPaths here. Do validation in BE instead
+            if isinstance(coordinates, (TextCoordinates, AudioCoordinates)):
+                for non_geometric_range in coordinates.range:
+                    self.check_within_range(non_geometric_range.end)
             else:
                 self.check_within_range(frame)
 
@@ -960,23 +949,38 @@ def __lt__(self, other: ObjectInstance) -> bool:
         return self._object_hash < other._object_hash
 
 
-def check_coordinate_type(coordinates: Coordinates, ontology_object: Object) -> None:
+def check_coordinate_type(coordinates: Coordinates, ontology_object: Object, parent: Optional[LabelRowV2]) -> None:
     """
     Check if the coordinate type matches the expected type for the ontology object.
 
     Args:
         coordinates (Coordinates): The coordinates to check.
         ontology_object (Object): The ontology object to check against.
+        parent (LabelRowV2): The parent label row (if any) of the ontology object.
 
     Raises:
         LabelRowError: If the coordinate type does not match the expected type.
     """
-    expected_coordinate_type = ACCEPTABLE_COORDINATES_FOR_ONTOLOGY_ITEMS[ontology_object.shape]
-    if not isinstance(coordinates, expected_coordinate_type):
+    expected_coordinate_types = ACCEPTABLE_COORDINATES_FOR_ONTOLOGY_ITEMS[ontology_object.shape]
+    if all(
+        not isinstance(coordinates, expected_coordinate_type) for expected_coordinate_type in expected_coordinate_types
+    ):
         raise LabelRowError(
-            f"Expected a coordinate of type `{expected_coordinate_type}`, but got type `{type(coordinates)}`."
+            f"Expected coordinates of one of the following types: `{expected_coordinate_types}`, but got type `{type(coordinates)}`."
         )
 
+    # An ontology object with `Text` shape can have both coordinates `HtmlCoordinates` and `TextCoordinates`
+    # Therefore, we need to further check the file type, to ensure that `HtmlCoordinates` are only used for
+    # HTML files, and `TextCoordinates` are only used for plain text files.
+    if isinstance(coordinates, TextCoordinates):
+        if parent is not None and parent == "text/html":
+            raise LabelRowError(f"Expected coordinates of type {HtmlCoordinates}`, but got type `{type(coordinates)}`.")
+    elif isinstance(coordinates, HtmlCoordinates):
+        if parent is not None and parent.file_type != "text/html":
+            raise LabelRowError(
+                "For non-html labels, ensure the `range` property " "is set when instantiating the TextCoordinates."
+            )
+
 
 class DynamicAnswerManager:
     """

tests/objects/data/audio_labels.py

@@ -39,7 +39,7 @@
                     "manualAnnotation": True,
                 },
             ],
-            "range": [[0, 1]],
+            "range": [[0, 0]],
             "createdBy": "user1Hash",
             "createdAt": "Tue, 05 Nov 2024 09:41:37 ",
             "lastEditedBy": "user1Hash",
@@ -71,7 +71,7 @@
                     "manualAnnotation": True,
                 },
             ],
-            "range": [[0, 1]],
+            "range": [[0, 0]],
             "createdBy": "user1Hash",
             "createdAt": "Tue, 05 Nov 2024 09:41:37 ",
             "lastEditedBy": "user1Hash",
@@ -101,7 +101,7 @@
                     "manualAnnotation": True,
                 },
             ],
-            "range": [[0, 1]],
+            "range": [[0, 0]],
             "createdBy": "user1Hash",
             "createdAt": "Tue, 05 Nov 2024 09:41:37 ",
             "lastEditedBy": "user1Hash",