refactor #2167
refactor #2167
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the BoxMOT detector interface by introducing a standardized, modular architecture with a unified Detector base class. The changes replace model-specific "strategy" classes with dedicated detector implementations that follow a consistent preprocess → process → postprocess pipeline.
- Introduces a new
Detectorbase class providing a unified interface across all detector types - Migrates YOLOX, Ultralytics, and RF-DETR to the new architecture with standalone detector classes
- Maintains backward compatibility through legacy interface functions while deprecating the old strategy pattern
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| boxmot/engine/detectors/base.py | New base class implementing the detector interface with resolve_image utility |
| boxmot/engine/detectors/yolox_detector.py | New YOLOX detector implementation replacing YoloXStrategy |
| boxmot/engine/detectors/ultralytics_detector.py | New Ultralytics detector implementation replacing UltralyticsStrategy |
| boxmot/engine/detectors/rfdetr_detector.py | New RF-DETR detector implementation replacing RFDETRStrategy |
| boxmot/engine/detectors/init.py | Updated to export new detector classes while maintaining legacy interface |
| boxmot/engine/detectors/yolox.py | Removed old YoloXStrategy implementation |
| boxmot/engine/detectors/rfdetr.py | Removed old RFDETRStrategy implementation |
| boxmot/engine/detectors/yolo_interface.py | Removed old YoloInterface abstract base class |
| boxmot/engine/detectors/README.md | Comprehensive documentation for the new interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Determine YOLOX model type from filename.""" | ||
| for key in YOLOX_ZOO.keys(): | ||
| if Path(key).stem in str(path.name): | ||
| return str(Path(key).with_suffix("")) |
There was a problem hiding this comment.
The call to Path(key).with_suffix('') is redundant. Since key already has .pt extension and we're extracting the stem, this can be simplified to return Path(key).stem which is clearer and more direct.
| return str(Path(key).with_suffix("")) | |
| return Path(key).stem |
| self.imgsz = [imgsz, imgsz] | ||
| else: | ||
| vals = imgsz if isinstance(imgsz, (list, tuple)) else (imgsz,) | ||
| w, h = (vals * 2)[:2] |
There was a problem hiding this comment.
When imgsz is already a list/tuple with 2 elements, the expression (vals * 2)[:2] will unnecessarily duplicate and then slice. For example, if vals = [640, 480], this produces [640, 480, 640, 480][:2] = [640, 480]. While the result is correct, if vals has only 1 element like (640,), the result is [640, 640]. Consider using w, h = (list(vals) + list(vals))[:2] or better yet, check the length explicitly for clarity: w, h = (vals[0], vals[1]) if len(vals) >= 2 else (vals[0], vals[0]).
| w, h = (vals * 2)[:2] | |
| w, h = (vals[0], vals[1]) if len(vals) >= 2 else (vals[0], vals[0]) |
| self.imgsz = imgsz | ||
| else: | ||
| vals = imgsz if isinstance(imgsz, (list, tuple)) else (imgsz,) | ||
| self.imgsz = vals[0] # Ultralytics uses single value |
There was a problem hiding this comment.
This code will raise an IndexError if imgsz is an empty list or tuple. Add a check: self.imgsz = vals[0] if vals else 640 or validate that vals is non-empty before accessing vals[0].
| self.imgsz = vals[0] # Ultralytics uses single value | |
| self.imgsz = vals[0] if vals else 640 # Ultralytics uses single value, default to 640 if empty |
| def _get_model_type(self, path: Path): | ||
| """Determine YOLOX model type from filename.""" | ||
| for key in YOLOX_ZOO.keys(): | ||
| if Path(key).stem in str(path.name): |
There was a problem hiding this comment.
Converting path.name to string is unnecessary since path.name is already a string. Simplify to if Path(key).stem in path.name:.
| if Path(key).stem in str(path.name): | |
| if Path(key).stem in path.name: |
|
|
||
| ## Examples | ||
|
|
||
| See `examples/detector_usage_example.py` for complete working examples including: |
There was a problem hiding this comment.
The documentation references examples/detector_usage_example.py but this file is not included in the PR changes. Verify that this example file exists or will be added, otherwise users following the documentation may encounter a broken reference.
boxmot/engine/detectors/rfdetr.py
Outdated
| from typing import Union | ||
|
|
There was a problem hiding this comment.
Import of 'Union' is not used.
| from typing import Union |
boxmot/engine/detectors/rfdetr.py
Outdated
| from rfdetr.util.coco_classes import COCO_CLASSES | ||
| RFDETR_AVAILABLE = True | ||
| except ImportError: | ||
| RFDETR_AVAILABLE = False | ||
| COCO_CLASSES = {} |
There was a problem hiding this comment.
Import of 'COCO_CLASSES' is not used.
| from rfdetr.util.coco_classes import COCO_CLASSES | |
| RFDETR_AVAILABLE = True | |
| except ImportError: | |
| RFDETR_AVAILABLE = False | |
| COCO_CLASSES = {} | |
| RFDETR_AVAILABLE = True | |
| except ImportError: | |
| RFDETR_AVAILABLE = False |
No description provided.