Add keypoints to random sized b box safe crop #1824
Conversation
Reviewer's Guide by SourceryThis pull request refactors the cropping transformations in the Albumentations library to include keypoints in the transformations. It introduces a new base class File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ternaus - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
| def get_params(self) -> dict[str, float]: | ||
| return {"h_start": random.random(), "w_start": random.random()} | ||
| def get_params_dependent_on_targets(self, params: dict[str, Any]) -> dict[str, tuple[int, int, int, int]]: |
There was a problem hiding this comment.
issue (bug_risk): Check for the presence of 'image' key in params.
It would be prudent to add a check to ensure that the 'image' key exists in the params dictionary before accessing it. This can prevent potential KeyError exceptions.
| "crop_and_pad", | ||
| "crop_and_pad_bbox", | ||
| "crop_and_pad_keypoint", | ||
| ] | ||
|
|
||
|
|
||
| def get_random_crop_coords( | ||
| def get_crop_coords( |
There was a problem hiding this comment.
suggestion: Consider adding validation for input parameters.
Adding validation checks for the input parameters (e.g., ensuring crop_height and crop_width are positive and within the bounds of height and width) can make the function more robust and prevent potential errors.
| def get_crop_coords( | |
| def get_crop_coords( | |
| height: int, | |
| width: int, | |
| crop_height: int, | |
| crop_width: int | |
| ): | |
| if crop_height <= 0 or crop_width <= 0: | |
| raise ValueError("Crop dimensions must be positive.") | |
| if crop_height > height or crop_width > width: | |
| raise ValueError("Crop dimensions must be within the bounds of the image dimensions.") |
| x1, y1 = crop_coords[:2] | ||
| cropped_bbox = x_min - x1, y_min - y1, x_max - x1, y_max - y1 | ||
| return cast(BoxInternalType, normalize_bbox(cropped_bbox, crop_height, crop_width)) | ||
| crop_height = crop_coords[3] - crop_coords[1] |
There was a problem hiding this comment.
suggestion: Consider adding a check for valid crop coordinates.
Before calculating crop_height and crop_width, it would be beneficial to add a check to ensure that crop_coords are valid and within the expected range.
| crop_height = crop_coords[3] - crop_coords[1] | |
| if not (0 <= crop_coords[0] < crop_coords[2] and 0 <= crop_coords[1] < crop_coords[3]): | |
| raise ValueError("Invalid crop coordinates") | |
| crop_height = crop_coords[3] - crop_coords[1] |
| @@ -0,0 +1,112 @@ | |||
| import numpy as np | |||
|
|
|||
| from tests.utils import set_seed | |||
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| params.update({"x_min": x_min, "x_max": x_max, "y_min": y_min, "y_max": y_max}) | ||
| crop_coords = x_min, y_min, x_max, y_max | ||
|
|
||
| params.update({"crop_coords": crop_coords}) |
There was a problem hiding this comment.
suggestion (code-quality): Add single value to dictionary directly rather than using update() (simplify-dictionary-update)
| params.update({"crop_coords": crop_coords}) | |
| params["crop_coords"] = crop_coords |
| erosive_h = int(image_height * (1.0 - self.erosion_rate)) | ||
| crop_height = image_height if erosive_h >= image_height else random.randint(erosive_h, image_height) | ||
|
|
||
| crop_width = int(crop_height * image_width / image_height) | ||
| h_start = random.random() | ||
| w_start = random.random() | ||
|
|
||
| crop_coords = fcrops.get_crop_coords(image_height, image_width, crop_height, crop_width, h_start, w_start) | ||
|
|
||
| return {"crop_coords": crop_coords} |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
Fixes #1765
Summary by Sourcery
This pull request refactors crop-related transformations to use a common base class, adds support for keypoints in BBoxSafeRandomCrop and RandomSizedBBoxSafeCrop, and updates the documentation and tests accordingly.