Refactor asserts and todos#2313
Draft
vikassaini77 wants to merge 25 commits into
Draft
Conversation
Updated README to improve formatting and structure.
Updated links, section headers, and example code paths in README.
|
|
Member
@vikassaini77 mind check pls ^^ 🦝 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors multiple runtime validations by replacing assert statements with explicit TypeError/ValueError raises, updates README formatting/structure, and adds a basic allowlist check before executing subprocess commands in an example script.
Changes:
- Replaced
assert-based validations with explicit exceptions across image utilities, keypoint annotators, VLM parsing, NMS utilities, converters, and line-zone logic. - Updated
README.mdstructure (Table of Contents, heading capitalization, example placeholders, and minor HTML fixes). - Added a command allowlist guard before
subprocess.run()inexamples/time_in_zone/scripts/stream_from_file.py.
Holistic assessment (n/5):
- Code quality: 3/5
- Testing: 2/5
- Documentation: 2/5
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/supervision/utils/image.py | Replaces assert type/invariant checks with explicit exceptions in image helpers. |
| src/supervision/key_points/annotators.py | Converts assert scene type checks to TypeError in annotators. |
| src/supervision/detection/vlm.py | Replaces assert validations with exceptions in Florence-2 parsing. |
| src/supervision/detection/utils/iou_and_nms.py | Replaces assert validations with ValueError in IoU/NMS utilities. |
| src/supervision/detection/utils/converters.py | Replaces assert validations with ValueError in mask_to_rle. |
| src/supervision/detection/line_zone.py | Converts internal assert invariants into explicit ValueErrors. |
| src/supervision/detection/core.py | Replaces assert validations with exceptions in VLM dispatch + NMS/NMM paths. |
| README.md | Adds a Table of Contents and cleans up headings/examples/HTML. |
| examples/time_in_zone/scripts/stream_from_file.py | Adds an allowlist check before running subprocess commands. |
Comment on lines
+87
to
+89
| if command[0] not in ["docker", "ffmpeg"]: | ||
| raise ValueError(f"Command {command[0]} not allowed") | ||
| process = subprocess.run(command) # noqa: S603 |
Comment on lines
+27
to
+38
| ## 📑 Table of Contents | ||
|
|
||
| - [👋 Hello](#-hello) | ||
| - [💻 Install](#-install) | ||
| - [🔥 Quickstart](#-quickstart) | ||
| - [Models](#models) | ||
| - [Annotators](#annotators) | ||
| - [Datasets](#datasets) | ||
| - [🎬 Tutorials](#-tutorials) | ||
| - [💜 Built with Supervision](#-built-with-supervision) | ||
| - [📚 Documentation](#-documentation) | ||
| - [🏆 Contribution](#-contribution) |
Comment on lines
+568
to
+571
| if not isinstance(result, str): | ||
| raise TypeError( | ||
| f"Expected string as <REGION_TO_CATEGORY> result, got {type(result)}" | ||
| ) |
Comment on lines
+730
to
+734
| if not 0 <= iou_threshold <= 1: | ||
| raise ValueError( | ||
| "Value of `iou_threshold` must be in the closed range from 0 to 1, " | ||
| f"{iou_threshold} given." | ||
| ) |
Comment on lines
+824
to
+828
| if not 0 <= iou_threshold <= 1: | ||
| raise ValueError( | ||
| "Value of `iou_threshold` must be in the closed range from 0 to 1, " | ||
| f"{iou_threshold} given." | ||
| ) |
Comment on lines
+2504
to
+2516
| if self.confidence is None: | ||
| raise ValueError( | ||
| "Detections confidence must be given for NMS to be executed." | ||
| ) | ||
|
|
||
| if class_agnostic: | ||
| predictions = np.hstack((self.xyxy, self.confidence.reshape(-1, 1))) | ||
| else: | ||
| assert self.class_id is not None, ( | ||
| "Detections class_id must be given for NMS to be executed. If you" | ||
| " intended to perform class agnostic NMS set class_agnostic=True." | ||
| ) | ||
| if self.class_id is None: | ||
| raise ValueError( | ||
| "Detections class_id must be given for NMS to be executed. If you" | ||
| " intended to perform class agnostic NMS set class_agnostic=True." | ||
| ) |
Comment on lines
+2584
to
+2596
| if self.confidence is None: | ||
| raise ValueError( | ||
| "Detections confidence must be given for NMM to be executed." | ||
| ) | ||
|
|
||
| if class_agnostic: | ||
| predictions = np.hstack((self.xyxy, self.confidence.reshape(-1, 1))) | ||
| else: | ||
| assert self.class_id is not None, ( | ||
| "Detections class_id must be given for NMM to be executed. If you" | ||
| " intended to perform class agnostic NMM set class_agnostic=True." | ||
| ) | ||
| if self.class_id is None: | ||
| raise ValueError( | ||
| "Detections class_id must be given for NMM to be executed. If you" | ||
| " intended to perform class agnostic NMM set class_agnostic=True." | ||
| ) |
Comment on lines
1153
to
+1157
| ... ) | ||
| >>> keep | ||
| array([ True, False]) | ||
| """ | ||
| assert 0 <= iou_threshold <= 1, ( | ||
| "Value of `iou_threshold` must be in the closed range from 0 to 1, " | ||
| f"{iou_threshold} given." | ||
| ) | ||
| if not 0 <= iou_threshold <= 1: |
Comment on lines
+694
to
+697
| if mask.ndim != 2: | ||
| raise ValueError("Input mask must be 2D") | ||
| if mask.size == 0: | ||
| raise ValueError("Input mask cannot be empty") |
Member
|
@vikassaini77 pls follow the contribution guidelines. In particular, it means you shall write PR description why it does and how it fixes or improves this package, then you have to sign the CLA (or provide a screenshot of what you did), otherwise, we cannot move forward, even I see based on your changes a value in your contribution 🦝 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before submitting
Description
Type of Change
Motivation and Context
Closes #(issue)
Changes Made
Testing
Google Colab (optional)
Colab link:
Screenshots/Videos (optional)
Additional Notes