-
Notifications
You must be signed in to change notification settings - Fork 2
Add eccentricity calculations #57
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
base: main
Are you sure you want to change the base?
Conversation
[Pre-review comment] It is unlikely that we will merge this branch, due to some hard-coded design decisions (e.g. the aforementioned manual selection of erosion/dilation kernels). When I review this, I will provide 2 types of feedback:
|
dilated_mask = cv2.dilate(eroded_mask, kernel, iterations=1) | ||
contours, _ = cv2.findContours(dilated_mask, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE) | ||
|
||
# if len(contours) > 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.
Please remove the commented-out code if it's not going to be part of the final version.
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.
Looks like the code is likely doing what you want it to.
2 comments for future generalization.
3 comments on docstrings/better descriptions.
1 question about a potential bug/handling of missing data.
@@ -119,3 +126,57 @@ def calculate_moments(contour_list): | |||
render = np.zeros([frame_size, frame_size, 1], dtype=np.uint8) | |||
_ = cv2.drawContours(render, contour_list, -1, [1], -1) | |||
return cv2.moments(render) | |||
|
|||
def calculate_contours(self, contour_list): |
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.
Function name/docstring could use some additional specificity.
calculate_contours
doesn't quite describe what this function does. The function appears to do the following:- Render contours provided
- Erode and dilate render
- Calculate new "filtered" contours
- Docstring mismatches almost all fields
- Summary incorrect
- parameters don't include
contour_list
and include 3 other values - return listed as
none
, but function has return
mask = np.zeros(frame_size, dtype=np.uint8) | ||
cv2.drawContours(mask, contour_list, -1, (1), thickness=cv2.FILLED) | ||
kernel = np.ones((7, 7), np.uint8) | ||
eroded_mask = cv2.erode(mask, kernel, iterations=1) |
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.
[Future work]
Erosion/dilation parameters should likely be somehow attached to pixel information.
Here, Lucia uses a 7x7 kernel with 1 iteration with the intent to remove the tail for her dataset (OFA, similar to https://doi.org/10.7910/DVN/SAPNJG). If we want to generalize this, we should probably get an average thickness of tail (in cm) and determine the kernel size via the number of pixel radius to adequately erode a feature of that size.
:param color: color of segmentation contours rendered on the GUI. | ||
:return: None | ||
""" | ||
frame_size = [800, 800] |
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.
[Future work]
Frame size is arbitrary and while 800x800 will work for this data, it should probably depend on max of contour data rather than hard-coded values. Would need to think a bit more, but may need a buffer edge with morphological filters (although erode should only shrink, should be fine...).
@@ -25,6 +25,8 @@ def __init__(self, poses: PoseEstimation, identity: int, | |||
self._moments = np.zeros((self._poses.num_frames, len(self._moment_keys)), dtype=np.float32) | |||
self._seg_data = self._poses.get_segmentation_data(identity) | |||
self._seg_flags = self._poses.get_segmentation_flags(identity) | |||
self._contours = [None] * self._poses._num_frames |
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.
Name of attribute should better describe the content. These are morphologically filtered contours, as opposed to "contours" (already present in self._seg_data
)
@@ -62,6 +66,9 @@ def get_moment(self, frame, key): | |||
""" | |||
key_idx = self._moment_keys.index(key) | |||
return self._moments[frame, key_idx] | |||
|
|||
def get_contours(self, frame): |
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.
Missing docstring
@@ -86,6 +89,17 @@ def per_frame(self, identity: int) -> np.ndarray: | |||
euler_number[frame] = np.sum(contour_flags[:len(contour_list)]==1)-np.sum(contour_flags[:len(contour_list)]==0) | |||
hole_area_ratio[frame] = (hole_areas * self._pixel_scale**2)/self._moment_cache.get_moment(frame, 'm00') | |||
|
|||
# CALCULATE ECCENTRICITY | |||
contours = self._moment_cache.get_contours(frame) | |||
if len(contours) > 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.
How does this handle missing segmentation (e.g. the None
initialization here)?
You may also need to check if contours is not None and ...
.
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.
Nice changes, seems like you cleaned up the commented out code + added missing docstrings from Brian and Gautam's comments. I second Brian's assessment on not merging this branch at this stage
Note: I manually chose a kernel for erosion/dilation, which probably isn't the smartest but it worked for the videos I think