Aadi/pymotmetrics #22
Conversation
only get visual features once
combine annotation and saving to single script for memory constraints
Co-authored-by: aaprasad <aaprasad.ucsd.edu> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates focus on enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Actionable comments outside the diff hunks (2)
biogtr/datasets/microscopy_dataset.py (1)
Line range hint
86-86: Replace lambda with a function definition for better readability and maintainability.- parser = lambda x: data_utils.parse_synthetic(x, source=source) + def parser(x): + return data_utils.parse_synthetic(x, source=source)Using explicit function definitions instead of lambda expressions can improve readability, especially when the function is non-trivial or used in multiple places.
biogtr/datasets/data_utils.py (1)
Line range hint
342-342: Replace the bareexceptwith more specific exception handling.Using a bare
exceptclause can catch unexpected exceptions, making debugging difficult. It is recommended to catch specific exceptions to avoid hiding bugs. For example:- except Exception as e: + except IndexError as e:This change ensures that only specific, anticipated errors are caught, making the code more robust and maintainable.
biogtr/data_structures.py
Outdated
| if not isinstance(crop, torch.Tensor): | ||
| self._features = torch.tensor(features) | ||
| else: | ||
| self._features = features |
There was a problem hiding this comment.
There is a potential bug in the conditional check for crop. It checks if crop is not a tensor twice, which seems to be a copy-paste error. The second check should be for features.
- if not isinstance(crop, torch.Tensor):
+ if not isinstance(features, torch.Tensor):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if not isinstance(crop, torch.Tensor): | |
| self._features = torch.tensor(features) | |
| else: | |
| self._features = features | |
| if not isinstance(features, torch.Tensor): | |
| self._features = torch.tensor(features) | |
| else: | |
| self._features = features |
biogtr/data_structures.py
Outdated
| try: | ||
| self._video = video_filename | ||
| except ValueError: | ||
| self._video = video_filename |
There was a problem hiding this comment.
The exception handling in the video setter seems unnecessary because the assignment operation does not raise an exception. Consider removing the try-except block.
- try:
self._video = video_filename
- except ValueError:
- self._video = video_filenameCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| try: | |
| self._video = video_filename | |
| except ValueError: | |
| self._video = video_filename | |
| self._video = video_filename |
| def forward(self, frames: list[Frame], query_frame: int = None): | ||
| """Execute forward pass of GTR Model to get asso matrix. | ||
|
|
||
| Args: | ||
| instances: List of dicts from chunk containing crops of objects + gt label info | ||
| all_instances: List of dicts containing crops of objects + gt label info. Used for stitching together full trajectory | ||
| frames: List of Frames from chunk containing crops of objects + gt label info | ||
| query_frame: Frame index used as query for self attention. Only used in sliding inference where query frame is the last frame in the window. | ||
|
|
||
| Returns: | ||
| An N_T x N association matrix | ||
| """ | ||
| # Extract feature representations with pre-trained encoder. | ||
| for frame in instances: | ||
| if (frame["num_detected"] > 0).item(): | ||
| z = self.visual_encoder(frame["crops"]) | ||
| frame["features"] = z | ||
| for frame in frames: | ||
| if frame.has_instances(): | ||
| if not frame.has_features(): | ||
| crops = frame.get_crops() | ||
| z = self.visual_encoder(crops) | ||
|
|
||
| for i, z_i in enumerate(z): | ||
| frame.instances[i].features = z_i | ||
|
|
||
| # Extract association matrix with transformer. | ||
| if self.transformer.return_embedding: | ||
| asso_preds, emb = self.transformer(instances, query_frame=query_frame) | ||
| else: | ||
| asso_preds = self.transformer(instances, query_frame=query_frame) | ||
| asso_preds, emb = self.transformer(frames, query_frame=query_frame) | ||
|
|
||
| return (asso_preds, emb) if self.transformer.return_embedding else asso_preds | ||
| return asso_preds, emb |
There was a problem hiding this comment.
Refactor the forward method to improve readability and performance.
- for frame in frames:
- if frame.has_instances():
- if not frame.has_features():
- crops = frame.get_crops()
- z = self.visual_encoder(crops)
- for i, z_i in enumerate(z):
- frame.instances[i].features = z_i
+ for frame in filter(lambda f: f.has_instances() and not f.has_features(), frames):
+ crops = frame.get_crops()
+ features = self.visual_encoder(crops)
+ for instance, feature in zip(frame.instances, features):
+ instance.features = featureThis refactoring uses Python's built-in filter function to streamline the checks for frames that have instances but not features, reducing the depth of nested conditions. It also pairs instances with their corresponding features directly using zip, enhancing clarity and reducing the risk of indexing errors.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def forward(self, frames: list[Frame], query_frame: int = None): | |
| """Execute forward pass of GTR Model to get asso matrix. | |
| Args: | |
| instances: List of dicts from chunk containing crops of objects + gt label info | |
| all_instances: List of dicts containing crops of objects + gt label info. Used for stitching together full trajectory | |
| frames: List of Frames from chunk containing crops of objects + gt label info | |
| query_frame: Frame index used as query for self attention. Only used in sliding inference where query frame is the last frame in the window. | |
| Returns: | |
| An N_T x N association matrix | |
| """ | |
| # Extract feature representations with pre-trained encoder. | |
| for frame in instances: | |
| if (frame["num_detected"] > 0).item(): | |
| z = self.visual_encoder(frame["crops"]) | |
| frame["features"] = z | |
| for frame in frames: | |
| if frame.has_instances(): | |
| if not frame.has_features(): | |
| crops = frame.get_crops() | |
| z = self.visual_encoder(crops) | |
| for i, z_i in enumerate(z): | |
| frame.instances[i].features = z_i | |
| # Extract association matrix with transformer. | |
| if self.transformer.return_embedding: | |
| asso_preds, emb = self.transformer(instances, query_frame=query_frame) | |
| else: | |
| asso_preds = self.transformer(instances, query_frame=query_frame) | |
| asso_preds, emb = self.transformer(frames, query_frame=query_frame) | |
| return (asso_preds, emb) if self.transformer.return_embedding else asso_preds | |
| return asso_preds, emb | |
| def forward(self, frames: list[Frame], query_frame: int = None): | |
| """Execute forward pass of GTR Model to get asso matrix. | |
| Args: | |
| frames: List of Frames from chunk containing crops of objects + gt label info | |
| query_frame: Frame index used as query for self attention. Only used in sliding inference where query frame is the last frame in the window. | |
| Returns: | |
| An N_T x N association matrix | |
| """ | |
| # Extract feature representations with pre-trained encoder. | |
| for frame in filter(lambda f: f.has_instances() and not f.has_features(), frames): | |
| crops = frame.get_crops() | |
| features = self.visual_encoder(crops) | |
| for instance, feature in zip(frame.instances, features): | |
| instance.features = feature | |
| asso_preds, emb = self.transformer(frames, query_frame=query_frame) | |
| return asso_preds, emb |
| def pose_bbox(points: np.ndarray, bbox_size: Union[tuple[int], int]) -> torch.Tensor: | ||
| """Calculate bbox around instance pose. | ||
|
|
||
| Args: | ||
| instance: a labeled instance in a frame, | ||
| padding: the amount to pad around the pose crop | ||
| im_shape: the size of the original image in (w,h) | ||
| bbox_size: size of bbox either an int indicating square bbox or in (x,y) | ||
|
|
||
| Returns: | ||
| Bounding box in [y1, x1, y2, x2] format. | ||
| """ | ||
| w, h = im_shape | ||
| if isinstance(bbox_size, int): | ||
| bbox_size = (bbox_size, bbox_size) | ||
| # print(points) | ||
| minx = np.nanmin(points[:, 0], axis=-1) | ||
| miny = np.nanmin(points[:, -1], axis=-1) | ||
| minpoints = np.array([minx, miny]).T | ||
|
|
||
| points = torch.Tensor([[p.x, p.y] for p in instance.points]) | ||
| maxx = np.nanmax(points[:, 0], axis=-1) | ||
| maxy = np.nanmax(points[:, -1], axis=-1) | ||
| maxpoints = np.array([maxx, maxy]).T | ||
|
|
||
| min_x = max(torch.nanmin(points[:, 0]) - padding, 0) | ||
| min_y = max(torch.nanmin(points[:, 1]) - padding, 0) | ||
| max_x = min(torch.nanmax(points[:, 0]) + padding, w) | ||
| max_y = min(torch.nanmax(points[:, 1]) + padding, h) | ||
| c = (minpoints + maxpoints) / 2 | ||
|
|
||
| bbox = torch.Tensor([min_y, min_x, max_y, max_x]) | ||
| bbox = torch.Tensor( | ||
| [ | ||
| c[-1] - bbox_size[-1] / 2, | ||
| c[0] - bbox_size[0] / 2, | ||
| c[-1] + bbox_size[-1] / 2, | ||
| c[0] + bbox_size[0] / 2, | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Consider using explicit indexing for clarity.
- c = (minpoints + maxpoints) / 2
- bbox = torch.Tensor(
- [
- c[-1] - bbox_size[-1] / 2,
- c[0] - bbox_size[0] / 2,
- c[-1] + bbox_size[-1] / 2,
- c[0] + bbox_size[0] / 2,
- ]
- )
+ cx, cy = (minpoints + maxpoints) / 2
+ height, width = bbox_size
+ bbox = torch.Tensor([cy - height / 2, cx - width / 2, cy + height / 2, cx + width / 2])This change makes the code easier to understand and maintain by explicitly naming the dimensions and coordinates.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def pose_bbox(points: np.ndarray, bbox_size: Union[tuple[int], int]) -> torch.Tensor: | |
| """Calculate bbox around instance pose. | |
| Args: | |
| instance: a labeled instance in a frame, | |
| padding: the amount to pad around the pose crop | |
| im_shape: the size of the original image in (w,h) | |
| bbox_size: size of bbox either an int indicating square bbox or in (x,y) | |
| Returns: | |
| Bounding box in [y1, x1, y2, x2] format. | |
| """ | |
| w, h = im_shape | |
| if isinstance(bbox_size, int): | |
| bbox_size = (bbox_size, bbox_size) | |
| # print(points) | |
| minx = np.nanmin(points[:, 0], axis=-1) | |
| miny = np.nanmin(points[:, -1], axis=-1) | |
| minpoints = np.array([minx, miny]).T | |
| points = torch.Tensor([[p.x, p.y] for p in instance.points]) | |
| maxx = np.nanmax(points[:, 0], axis=-1) | |
| maxy = np.nanmax(points[:, -1], axis=-1) | |
| maxpoints = np.array([maxx, maxy]).T | |
| min_x = max(torch.nanmin(points[:, 0]) - padding, 0) | |
| min_y = max(torch.nanmin(points[:, 1]) - padding, 0) | |
| max_x = min(torch.nanmax(points[:, 0]) + padding, w) | |
| max_y = min(torch.nanmax(points[:, 1]) + padding, h) | |
| c = (minpoints + maxpoints) / 2 | |
| bbox = torch.Tensor([min_y, min_x, max_y, max_x]) | |
| bbox = torch.Tensor( | |
| [ | |
| c[-1] - bbox_size[-1] / 2, | |
| c[0] - bbox_size[0] / 2, | |
| c[-1] + bbox_size[-1] / 2, | |
| c[0] + bbox_size[0] / 2, | |
| ] | |
| ) | |
| def pose_bbox(points: np.ndarray, bbox_size: Union[tuple[int], int]) -> torch.Tensor: | |
| """Calculate bbox around instance pose. | |
| Args: | |
| instance: a labeled instance in a frame, | |
| bbox_size: size of bbox either an int indicating square bbox or in (x,y) | |
| Returns: | |
| Bounding box in [y1, x1, y2, x2] format. | |
| """ | |
| if isinstance(bbox_size, int): | |
| bbox_size = (bbox_size, bbox_size) | |
| # print(points) | |
| minx = np.nanmin(points[:, 0], axis=-1) | |
| miny = np.nanmin(points[:, -1], axis=-1) | |
| minpoints = np.array([minx, miny]).T | |
| maxx = np.nanmax(points[:, 0], axis=-1) | |
| maxy = np.nanmax(points[:, -1], axis=-1) | |
| maxpoints = np.array([maxx, maxy]).T | |
| cx, cy = (minpoints + maxpoints) / 2 | |
| height, width = bbox_size | |
| bbox = torch.Tensor([cy - height / 2, cx - width / 2, cy + height / 2, cx + width / 2]) |
| For some reason it screws up `test_training` by causing a device error | ||
| """ | ||
|
|
There was a problem hiding this comment.
Investigate and resolve the root cause of the device error instead of commenting out code.
Commenting out code is a temporary solution that can lead to technical debt. It's important to investigate the root cause of the device error mentioned and resolve it properly to ensure the robustness of the test suite.
Summary by CodeRabbit
New Features
TrackQueueclass for managing tracking operations over sliding windows.Enhancements
Bug Fixes
Documentation
Refactor
Tests
Dependencies