Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions forte/common/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
# The index storing entry type in the internal entry data of DataStore.
ENTRY_TYPE_INDEX = 3

# The index storing the payload ID in internal entry data of DataStore
PAYLOAD_INDEX = 0

# The index storing entry type (specific to Link and Group type). It is saved
# in the `tid_idx_dict` in DataStore.
ENTRY_DICT_TYPE_INDEX = 0
Expand Down
69 changes: 56 additions & 13 deletions forte/data/ontology/top.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
PARENT_TID_INDEX,
CHILD_TID_INDEX,
MEMBER_TID_INDEX,
PAYLOAD_INDEX,
)

__all__ = [
Expand Down Expand Up @@ -72,8 +73,15 @@
make sure it available across the ontology system:
1. Create a new top level class that inherits from `Entry` or `MultiEntry`
2. Add the new class to `SinglePackEntries` or `MultiPackEntries`
3. Register a new method in `DataStore`: `add_<new_entry>_raw()`
4. Insert a new conditional branch in `EntryConverter.save_entry_object()`
3. Insert a new conditional branch in `EntryConverter.save_entry_object()`
4. Decide two main attributes which will qualify as your `attribute_data`
parameters. These parameters will be passes in your branch of
`EntryConverter.save_entry_object()`. If there are no such parameters,
you can pass None
5. add `getter` and `setter` functions to update `attribute_data` parameters
if you have any
6. If additional attributes are required, make the class a `dataclass` and set
`dataclass` attributes.
"""


Expand Down Expand Up @@ -879,7 +887,9 @@ def image(self):
"Cannot get image because image annotation is not "
"attached to any data pack."
)
return self.pack.get_image_array(self._image_payload_idx)
return self.pack.get_payload_data_at(
Modality.Image, self._image_payload_idx
)

@property
def max_x(self):
Expand Down Expand Up @@ -1052,12 +1062,35 @@ def __init__(self, pack: PackType, image_payload_idx: int = 0):
else:
self._image_payload_idx = image_payload_idx

@property
def image_payload_idx(self):
r"""Getter function of ``image_payload_idx``. The function will first try to
retrieve the image_payload_idx index from ``DataStore`` in ``self.pack``. If
this attempt fails, it will directly return the value in ``_image_payload_idx``.
"""
try:
self._image_payload_idx = self.pack.get_entry_raw(self.tid)[
PAYLOAD_INDEX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't use too many special const, we should simply store the payload details as attributes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Payload Index is the only main attribute for ImageAnnotation type entries (just like how Annotation has start and end indices, payload_index is stored in the first two indices of the DataStore entry. That's why I felt that could be the only constant we could add for ImageAnnotation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just try not to add additional consts here. DataStore has a system to add attributes and datastore can manage the attribute position. Can we simply consider Payload_index to be a regular attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Every Entry has two main attributes which occupy the first and second position in the datastore (for Annotation it is begin and end index). Similar, for ImageAnnotation and its subclasses, the two main attributes are payload_idx and None (Implying that it only has one main attribute, the extra None is just for consistency). That is the reason I stored the Payload index value in constants

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the None here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Payload Index is the only main attribute for ImageAnnotation type entries (just like how Annotation has start and end indices, payload_index is stored in the first two indices of the DataStore entry. That's why I felt that could be the only constant we could add for ImageAnnotation

Ok, so I think the reason I have the confusion is that: once Payload is in our system, then all other payload-dependent entries (specifically, Annotation, ImageAnnotation, AudioAnnotation) should have Payload associated with it too. Now we only modify it for ImageAnnotation in this PR, it feels a bit inconsistent to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I understand now. In that case, do you suggest we make the payload index a data class attribute? In this way we can have it in all three.

Also, None here has no special significance. The DataStore entry structure is that the first two elements are the two main attributes, followed by tid, entry type and data class attributes (if any). So in the case of ImageAnnotation, there is only one main attribute, so just to keep the structure consistent we have the second attribute as None. This is the method followed for other entries that have less than two main attributes as well (eg. Generics)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. So we should make all the three have a Payload field. I am not sure of what data class attribute means yet, but we need to make all of the consistent, but with two considerations:
    i. maybe it will make this PR too large?
    ii. we don't want to change any existing behavior, i.e., previously we don't have multiple payloads for each modality, thus all entries are retrieved without any payload index. To keep the same behavior, we should assume any data operation that does not use a payload index default to 0, i.e., get from the first payload or add to the first payload when payload index is not specified.
  2. Can you remind me what are two main attributes? I just wonder why it could be None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1(i) I feel that changing the structure for all entry types to include payload index would be out of the scope of this PR.
1(ii) I believe this behavior is already being displayed atleast in the ImageAnnotation type entries.

2 So this is a structure that is followed by all DataStore entries. The first two elements of the list are the two most common attributes of that entry (eg. begin and end index for Annotation). This is then followed by the remaining attributes like tid, type_name, etc. The structure is such that we need strictly TWO elements of common attributes in the beginning. But in the case of ImageAnnotation, there is only one such element (payload_index). Thus, to keep the structure consistent, we keep the second element as None.

Copy link
Member

@hunterhector hunterhector Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1(i) I feel that changing the structure for all entry types to include payload index would be out of the scope of this PR.

Sure, as long as we can ensure the payload index is handled consistently, and we should create issues to track this for the next step.

1(ii) I believe this behavior is already being displayed at least in the ImageAnnotation type entries.

Yeah this is more like a reminder.

2 So this is a structure that is followed by all DataStore entries. The first two elements of the list are the two most common attributes of that entry (eg. begin and end index for Annotation). This is then followed by the remaining attributes like tid, type_name, etc. The structure is such that we need strictly TWO elements of common attributes in the beginning. But in the case of ImageAnnotation, there is only one such element (payload_index). Thus, to keep the structure consistent, we keep the second element as None.

We actually can have different internal list structures for different entry types, I hope we don't have to strictly need two elements for each item.

]
except KeyError:
pass
return self._image_payload_idx

@image_payload_idx.setter
def image_payload_idx(self, val: int):
r"""Setter function of ``image_payload_idx``. The update will also be populated
into ``DataStore`` in ``self.pack``.
"""
self._image_payload_idx = val
self.pack.get_entry_raw(self.tid)[PAYLOAD_INDEX] = val

def compute_iou(self, other) -> int:
intersection = np.sum(np.logical_and(self.image, other.image))
union = np.sum(np.logical_or(self.image, other.image))
return intersection / union


@dataclass
class Box(Region):
"""
A box class with a center position and a box configuration.
Expand All @@ -1078,13 +1111,18 @@ class Box(Region):
width: the width of the box, the unit is one image array entry.
"""

_cy: int
_cx: int
_height: int
_width: int

def __init__(
self,
pack: PackType,
cy: int,
cx: int,
height: int,
width: int,
cy: int = 0,
cx: int = 0,
height: int = 1,
width: int = 1,
image_payload_idx: int = 0,
):
# assume Box is associated with Grids
Expand Down Expand Up @@ -1180,6 +1218,7 @@ def compute_iou(self, other):
return intersection / union


@dataclass
class BoundingBox(Box):
"""
A bounding box class that associates with image payload and grids and
Expand Down Expand Up @@ -1208,15 +1247,17 @@ class BoundingBox(Box):

"""

_grid_id: int

def __init__(
self,
pack: PackType,
height: int,
width: int,
grid_height: int,
grid_width: int,
grid_cell_h_idx: int,
grid_cell_w_idx: int,
height: int = 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only grid move to the class level? Note that it is not just about moving things to data store, but need to have a consistent approach too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that was the original implementation of BoundingBox. Do you feel that we should not create the Grid object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean you moved _grid_id to be a class variable. Do you understand how these class variable affect the attribute list?

I don't know what you mean by creeating the Grid object, but that should not be a problem for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you suggested, I am using the new implementation of CV ontologies (from @hepengfe 's branch) and making my changes on that so that merging is easier. I believe he has handled the Grid implementation there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, probably this PR is a bit intertwined with the Grid which makes me hard to judge the correctness right now.

width: int = 1,
grid_height: int = 1,
grid_width: int = 1,
grid_cell_h_idx: int = 0,
grid_cell_w_idx: int = 0,
image_payload_idx: int = 0,
):
self.grids = Grids(pack, grid_height, grid_width, image_payload_idx)
Expand All @@ -1228,6 +1269,8 @@ def __init__(
image_payload_idx,
)

self._grid_id = self.grids.tid


class Payload(Entry):
"""
Expand Down
Loading