Aadi/refactor-tracker #23
Conversation
specify n_devices from config
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## aadi-sample-efficiency #23 +/- ##
==========================================================
+ Coverage 75.65% 75.79% +0.14%
==========================================================
Files 24 24
Lines 1532 1541 +9
==========================================================
+ Hits 1159 1168 +9
Misses 373 373 ☔ View full report in Codecov by Sentry. |
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 update enhances the BioGTR framework, focusing on improved data handling, model inference, and configuration management. New data structures and datasets are introduced, and significant enhancements in tracking and visualization capabilities are implemented. The changes also include better error handling, memory management, and documentation to streamline the development and training processes. Changes
Poem
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: 9
Actionable comments outside the diff hunks (1)
biogtr/datasets/data_utils.py (1)
Line range hint
174-175: Avoid using bareexceptstatements.- except Exception as e: - print(e) - pass + except IndexError as e: + print(f"Error accessing data: {e}")Using a bare
exceptcan catch unexpected exceptions and make debugging difficult. It's better to catch specific exceptions that you expect might occur during normal operation. In this case, anIndexErrormight be more appropriate if the error is related to accessing list indices.
|
|
||
| if not self.persistent_tracking: | ||
| if self.verbose: | ||
| warnings.warn(f"Clearing Queue after tracking") |
There was a problem hiding this comment.
The warning message uses an f-string without placeholders. Consider using a regular string if no formatting is needed.
- warnings.warn(f"Clearing Queue after tracking")
+ warnings.warn("Clearing Queue after tracking")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.
| warnings.warn(f"Clearing Queue after tracking") | |
| warnings.warn("Clearing Queue after tracking") |
| anchor = self.anchor | ||
| else: | ||
| if self.verbose: | ||
| warnings.warn( |
There was a problem hiding this comment.
The warnings module is used but not imported, which will cause a runtime error.
+ import warningsCommittable 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.
| warnings.warn( | |
| import warnings | |
| warnings.warn( |
| for i, instance in enumerate(frames[batch_idx].instances): | ||
| if instance.pred_track_id == -1: | ||
| instance.pred_track_id = curr_track_id | ||
| curr_track += 1 |
There was a problem hiding this comment.
Undefined variable curr_track used. It seems there is a typo or missing initialization.
- curr_track += 1
+ curr_track_id += 1Committable 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.
| curr_track += 1 | |
| curr_track_id += 1 |
biogtr/inference/track_queue.py
Outdated
| except Exception as e: | ||
| print(f"Unable to end track due to {e}") |
There was a problem hiding this comment.
Handle exceptions more gracefully in the end_tracks method.
- except Exception as e:
- print(f"Unable to end track due to {e}")
- return False
+ except KeyError:
+ if self.verbose:
+ print(f"Track ID {track_id} not found in the queue.")
+ return FalseUsing a generic Exception is not recommended as it can mask other unexpected issues. It's better to catch specific exceptions (like KeyError for missing track IDs) to make the error handling clearer and more predictable.
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.
| except Exception as e: | |
| print(f"Unable to end track due to {e}") | |
| except KeyError: | |
| if self.verbose: | |
| print(f"Track ID {track_id} not found in the queue.") | |
| return False |
| print([[f.device for f in frame.get_features()] for frame in frames]) | ||
| raise (e) |
There was a problem hiding this comment.
Improve error handling in the forward method.
- except Exception as e:
- print([[f.device for f in frame.get_features()] for frame in frames])
- raise (e)
+ except RuntimeError as e:
+ print("Error during feature concatenation:", e)
+ raiseCatching a general Exception is too broad and can obscure the source of errors. It's better to catch more specific exceptions like RuntimeError, which could occur during tensor operations. Additionally, improve the error message to provide more context about the error.
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.
| print([[f.device for f in frame.get_features()] for frame in frames]) | |
| raise (e) | |
| print("Error during feature concatenation:", e) | |
| raise |
| def get_boxes_times(frames: List[Frame]) -> Tuple[torch.Tensor, torch.Tensor]: | ||
| """Extract the bounding boxes and frame indices from the input list of instances. | ||
|
|
||
| Args: | ||
| instances (List[Dict]): List of instance dictionaries | ||
| frames (List[Frame]): List of frame objects containing metadata and instances. | ||
|
|
||
| Returns: | ||
| Tuple[torch.Tensor, torch.Tensor]: A tuple of two tensors containing the | ||
| bounding boxes and corresponding frame | ||
| indices, respectively. | ||
| """ | ||
| boxes, times = [], [] | ||
| _, h, w = instances[0]["img_shape"].flatten() | ||
| _, h, w = frames[0].img_shape.flatten() | ||
|
|
||
| for fidx, instance in enumerate(instances): | ||
| bbox = deepcopy(instance["bboxes"]) | ||
| for fidx, frame in enumerate(frames): |
There was a problem hiding this comment.
Refactor the get_boxes_times function to efficiently handle Frame objects.
10 def get_boxes_times(frames: List[Frame]) -> Tuple[torch.Tensor, torch.Tensor]:
11 """Extract the bounding boxes and frame indices from the input list of instances.
12
13 Args:
14 frames (List[Frame]): List of frame objects containing metadata and instances.
15
16 Returns:
17 Tuple[torch.Tensor, torch.Tensor]: A tuple of two tensors containing the
18 bounding boxes and corresponding frame
19 indices, respectively.
20 """
21 boxes, times = [], []
22 _, h, w = frames[0].img_shape.flatten()
23
24 for fidx, frame in enumerate(frames):
25 bbox = deepcopy(frame.get_bboxes())
26 bbox[:, [0, 2]] /= w
27 bbox[:, [1, 3]] /= h
28
29 boxes.append(bbox)
30 times.append(torch.full((bbox.shape[0],), fidx))
31
32 boxes = torch.cat(boxes, dim=0) # N x 4
33 times = torch.cat(times, dim=0).to(boxes.device) # N
34 return boxes, timesEnsure that the function efficiently processes the Frame objects to extract bounding boxes and frame indices, optimizing for performance and memory usage.
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 get_boxes_times(frames: List[Frame]) -> Tuple[torch.Tensor, torch.Tensor]: | |
| """Extract the bounding boxes and frame indices from the input list of instances. | |
| Args: | |
| instances (List[Dict]): List of instance dictionaries | |
| frames (List[Frame]): List of frame objects containing metadata and instances. | |
| Returns: | |
| Tuple[torch.Tensor, torch.Tensor]: A tuple of two tensors containing the | |
| bounding boxes and corresponding frame | |
| indices, respectively. | |
| """ | |
| boxes, times = [], [] | |
| _, h, w = instances[0]["img_shape"].flatten() | |
| _, h, w = frames[0].img_shape.flatten() | |
| for fidx, instance in enumerate(instances): | |
| bbox = deepcopy(instance["bboxes"]) | |
| for fidx, frame in enumerate(frames): | |
| def get_boxes_times(frames: List[Frame]) -> Tuple[torch.Tensor, torch.Tensor]: | |
| """Extract the bounding boxes and frame indices from the input list of instances. | |
| Args: | |
| frames (List[Frame]): List of frame objects containing metadata and instances. | |
| Returns: | |
| Tuple[torch.Tensor, torch.Tensor]: A tuple of two tensors containing the | |
| bounding boxes and corresponding frame | |
| indices, respectively. | |
| """ | |
| boxes, times = [], [] | |
| _, h, w = frames[0].img_shape.flatten() | |
| for fidx, frame in enumerate(frames): | |
| bbox = deepcopy(frame.get_bboxes()) | |
| bbox[:, [0, 2]] /= w | |
| bbox[:, [1, 3]] /= h | |
| boxes.append(bbox) | |
| times.append(torch.full((bbox.shape[0],), fidx)) | |
| boxes = torch.cat(boxes, dim=0) # N x 4 | |
| times = torch.cat(times, dim=0).to(boxes.device) # N | |
| return boxes, times |
*Add better variable names for model and tracker
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Tests