-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Grids Ontology Improvement #827
Conversation
…x the initialization parameter requirement)
…o grid_improve
The implementation design considerations is:
Several questions about the design: |
Codecov Report
@@ Coverage Diff @@
## master #827 +/- ##
==========================================
+ Coverage 80.75% 80.81% +0.05%
==========================================
Files 254 254
Lines 19364 19574 +210
==========================================
+ Hits 15637 15818 +181
- Misses 3727 3756 +29
Continue to review full report at Codecov.
|
Usually, there are many regular ontologies(such as bounding box) and many images but only few grids. The number of grids actually needed only depends on the number of different image sizes and the number of grid configurations. My proposal is that New design: we implement a Grid interface such that only a collection of grids (probably tens of them) is needed to query-related data given image and grid-related parameters/regular ontology such as BoundingBox. Grid itself just like other ontology doesn't store image data. In addition, it is not bound to any other regular ontology. If this design is expected, we might not want it to inherit entry as it will create new ones for each DataPack. |
Good thoughts on limiting "number of grids", feels like a good idea, though I haven't considered everything about it.
I have a few suggestions on this detail though. You can already see the problem of making Instead, if you want to achieve this, we probably will make Grid a basic data structure used by the Forte DataPack system only. (i.e. Grid is more like a List, SortedList, TList or Adjacent Array, instead of an Entry). In DataPack, we can create some default Grids and users won't involve in this process, they are the underlying data structures that speed up certain computations only. Having {
"entry_name": "User.Defined.Type",
"entry_attributes":
{
"grid": "forte.data.core.grid",
}
} We can allow users to invoke some functions like computing the position of a particular bounding box with any grids, even the one they defined. However, these should be advanced and rare use cases for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To continue this PR, let's make Grid
a data structure, like Span
.
…o grid_improve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You can store
Grid
in the data store, but don't store them like other entries, because they are not. - I think you haven't handled (or at least haven't documented) the behavior of the edge cases well. How do we treat the pixels on the side where
y > c_h * self._height
. Let's make sure we have a common understanding of the behavior, test the edge cases and write the documentation for it
forte/data/data_store.py
Outdated
@@ -681,7 +679,7 @@ def _new_grid( | |||
tid: int = self._new_tid() if tid is None else tid | |||
entry: List[Any] | |||
|
|||
entry = [image_payload_idx, None, tid, type_name] | |||
entry = [None, None, tid, type_name] | |||
entry += self._default_attributes_for_type(type_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a Grid
is no longer an entry, why are we still storing them like entries?
forte/data/data_store.py
Outdated
@@ -883,7 +881,7 @@ def num_entries(self, entry_type_name: str) -> int: | |||
|
|||
def _add_entry_raw( | |||
self, | |||
entry_type: Type[Entry], | |||
entry_type: Union[Type[Entry], Type[Grid]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do this here, we basically don't store them together like other entries anymore.
forte/data/data_store.py
Outdated
@@ -1082,7 +1080,6 @@ def add_image_annotation_raw( | |||
def add_grid_raw( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't have add_grid_raw
image_height: int, | ||
image_width: int, | ||
): | ||
if height <= 0 or width <= 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we also check for image_height
forte/data/ontology/core.py
Outdated
self.c_h, self.c_w = ( | ||
image_height // self._height, | ||
image_width // self._width, | ||
) # compute the height and width of grid cells |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We round up the division here, but what's the implication? Does it mean some grid cells are larger? Or do we have residual values? Which ones are larger and what's the behavior? We need full documentation of these
] = img_arr[ | ||
h_idx * self.c_h : (h_idx + 1) * self.c_h, | ||
w_idx * self.c_w : (w_idx + 1) * self.c_w, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the behavior related to the residual (incomplete) grids on the edge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the residual grid cell (index range) will be smaller than other regular ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be explained in the docstring clearly.
forte/data/ontology/top.py
Outdated
@@ -1010,40 +876,89 @@ class Box(Region): | |||
pack: the container that this ``Box`` will be added to. | |||
image_payload_idx: the index of the image payload. If it's not set, | |||
it defaults to 0 which meaning it will load the first image payload. | |||
height: the height of the box, the unit is one image array entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users have no idea what "one image array entry" means
forte/data/ontology/top.py
Outdated
cy: the row index of the box center in the image array, | ||
the unit is one image array entry. | ||
the unit is one image array entry. If not set, the box center | ||
will be set to center of image (half height of the image). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to provide default values here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea I was thinking is that regular shape makes sense only when it has a reference point (box center here).
I am thinking two layers of conditions
- Whether it has a non-default reference point (maybe we don't need to set them to an actual value here).
- Whether it's associated with a grid. (It might or might not depending on the box use cases)
… with respector to Box.corners)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we will need a comprehensive example to showcase the power of Grid
(maybe in another PR).
grid cell center. | ||
3. When we predict a box, we will have the predicted box shape (height, | ||
width) and the offset between the box center and the grid cell center, | ||
then we can compute the box center. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create a standalone Box also for ground truth box or predicted box? Do we assume that the input parameters to a predicted box will be the box shape and gird center offset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In the first use case, only cy
and cx
are required to be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing about this PR is the documentation. It is a bit hard for me to understand the purpose of classes and functions. We need to create a simple tutorial to explain how these things work (and also include some examples in the docstring itself).
@mylibrar I also haven't checked the implementation details, please check that part.
forte/data/data_pack.py
Outdated
@@ -172,7 +170,9 @@ def __init__(self, pack_name: Optional[str] = None): | |||
self._data_store: DataStore = DataStore() | |||
self._entry_converter: EntryConverter = EntryConverter() | |||
self.image_annotations: List[ImageAnnotation] = [] | |||
self.grids: List[Grids] = [] | |||
self.payloads: List[np.ndarray] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an unused variable. And more, this make a new assumption that we store a list of payload in the format as np.ndarray
. Let's not do this now (at least not in this PR).
forte/data/data_pack.py
Outdated
self.grids: List[Grids] = [] | ||
self.payloads: List[np.ndarray] = [] | ||
|
||
self.grids: List[Grid] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not used in this file.
""" | ||
Regular grid with a grid configuration dependent on the image size. | ||
It is a data structure used to retrieve grid-related objects such as grid | ||
cells from the image. Grid itself doesn't store any data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the core functionality of a Grid
is used to index the image area so that we can easily compute the overlap of any two areas. Let's don't forget this.
Btw, is there any use case you find that we need to retrieve the image inside the grid cell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think bounding box prediction is usually based on only image on inside a grid cell (anchor box) in YOLO algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is it a convention or just a choice of a particular library (YOLO)?
- you will need to explain these things to the users. before that, can you make sure you explain this to the team so people understand what you are doing?
we compute the height and the width of grid cells. | ||
For example, if the image size (image_height,image_width) is (640, 480) | ||
and the grid shape (height, width) is (2, 3) | ||
the size of grid cells (self.c_h, self.c_w) will be (320, 160). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you might want to discuss how we handle the cases where the numbers are indivisible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed
forte/data/ontology/core.py
Outdated
The grid can be totally "free-form" that we don't initialize it with any | ||
image size and pass the image size directly into the method/operation on | ||
the fly. However, since the number of different image shapes are limited, | ||
we can bound one grid to one image size, and we can also do the image/grid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is still confusing, try to explain this to someone who never works on this and see if they understand. For example, why does one need to bound the grid to the image size? What are "size check"?
forte/data/ontology/top.py
Outdated
@property | ||
def cy(self) -> int: | ||
""" | ||
Compute and return row index of the box center in the image array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
units?
forte/data/ontology/top.py
Outdated
return (self._cy, self._cx) | ||
def box_center(self) -> Tuple[int, int]: | ||
""" | ||
Get the box center by using the property function cy() and cx(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't write implementation details in function docstring, users often won't care (and might get confused).
forte/data/ontology/top.py
Outdated
""" | ||
Get corners of box. | ||
Compute and return the corners of the box. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a corner? Which corners? Shape of the tuple?
|
||
@property | ||
def area(self): | ||
def area(self) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like area
should also exist in the upper classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True if the offset is set, False otherwise. | ||
""" | ||
return self._cy_offset is not None and self._cx_offset is not None | ||
|
||
|
||
class BoundingBox(Box): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what differences would a boundingbox have comparing to a box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I don't think a box need be associated with a grid but it can be associated with grid anyway. A bounding box are much more often associated with a grid due to the CV algorithm.
Closed as created a new PR #876 |
This PR fixes #817.
Description of changes
Grids
->Grid
and move it tocore.py
as a data structure rather than an ontologyBoundingBox
Box
based on three use cases.Box
Box
condition attributesself._is_grid_associated
self._is_box_center_set()
Possible influences of this PR.
Describe what are the possible side-effects of the code change.
Test Conducted
Describe what test cases are included for the PR.