Add copy paste #1820
Add copy paste #1820
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new 'CopyPaste' augmentation, updates the 'OverlayElements' class, and modifies related functions and classes to support the new functionality. It also includes comprehensive test cases to ensure the new features work as expected. 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: 5 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 12 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| return ("metadata_key",) | ||
|
|
||
|
|
||
| class CopyPaste(DualTransform): |
There was a problem hiding this comment.
suggestion: Clarify the purpose of the CopyPaste class.
The CopyPaste class lacks a detailed docstring explaining its purpose and usage. Adding a comprehensive docstring would improve code readability and maintainability.
| class CopyPaste(DualTransform): | |
| class CopyPaste(DualTransform): | |
| """ | |
| Apply Copy-Paste augmentation to the input data. | |
| This transformation is used to copy objects from one image and paste them onto another. | |
| Targets: image, mask, bboxes, keypoints | |
| """ |
| mask = cv2.resize(mask, (x_max - x_min, y_max - y_min), interpolation=cv2.INTER_NEAREST) | ||
| else: | ||
| mask = np.ones((y_max - y_min, x_max - x_min), dtype=np.uint8) | ||
| if overlay_height > image_height or overlay_width > image_width: |
There was a problem hiding this comment.
suggestion: Consider edge cases for overlay image resizing.
The current logic resizes the overlay image if its dimensions exceed the base image dimensions. Consider adding a check to handle cases where the overlay image is significantly larger, which might affect the quality of the transformation.
| if overlay_height > image_height or overlay_width > image_width: | |
| if overlay_height > image_height or overlay_width > image_width: | |
| if overlay_height > 2 * image_height or overlay_width > 2 * image_width: | |
| overlay = cv2.resize(overlay, (image_width, image_height), interpolation=cv2.INTER_AREA) | |
| else: | |
| overlay = cv2.resize(overlay, (image_width, image_height), interpolation=cv2.INTER_LINEAR) |
| metadata = params[self.metadata_key] | ||
| img_shape = params["image"].shape | ||
|
|
||
| if isinstance(metadata, list): |
There was a problem hiding this comment.
suggestion: Clarify handling of metadata as a list.
The code handles metadata as a list but does not specify how this list is expected to be structured. Clarifying this would help future maintainers understand the expected input format.
| if isinstance(metadata, list): | |
| if isinstance(metadata, list) and all(isinstance(md, dict) for md in metadata): |
| ) -> np.ndarray: | ||
| y_offset, x_offset = offset | ||
| if overlay_image.size == 0: |
There was a problem hiding this comment.
suggestion (testing): Check for empty overlay image.
The check for an empty overlay image is a good addition. Ensure that this condition is tested to avoid unexpected behavior.
| if overlay_image.size == 0: | |
| if overlay_image is None or overlay_image.size == 0: |
| return blended_image | ||
|
|
||
|
|
||
| def mask2bbox(binary_mask: np.ndarray) -> Tuple[int, int, int, int]: |
There was a problem hiding this comment.
suggestion: Consider edge cases for mask2bbox.
The mask2bbox function should handle cases where the mask is entirely zero. Ensure that the returned coordinates are valid and do not cause issues downstream.
| def mask2bbox(binary_mask: np.ndarray) -> Tuple[int, int, int, int]: | |
| def mask2bbox(binary_mask: np.ndarray) -> Tuple[int, int, int, int]: | |
| y_indices, x_indices = np.where(binary_mask) | |
| if y_indices.size == 0 or x_indices.size == 0: | |
| return 0, 0, 0, 0 |
| if augmentation_cls == A.OverlayElements: | ||
| data = { | ||
| "image": image_3ch, | ||
| "overlay_metadata": [], | ||
| "mask": mask_3ch, | ||
| } | ||
| data["overlay_metadata"] = [] | ||
| elif augmentation_cls == A.FromFloat: | ||
| data = { | ||
| "image": SQUARE_FLOAT_IMAGE, | ||
| "mask": mask_3ch, | ||
| } | ||
| else: | ||
| data = { | ||
| "image": image_3ch, | ||
| "mask": mask_3ch, | ||
| } | ||
| data["image"] = SQUARE_FLOAT_IMAGE | ||
| elif augmentation_cls == A.CopyPaste: | ||
| data["copypaste_metadata"] = [] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if augmentation_cls == A.OverlayElements: | ||
| data = { | ||
| "image": image, | ||
| "overlay_metadata": [], | ||
| } | ||
| else: | ||
| data = { | ||
| "image": image, | ||
| } | ||
| data["overlay_metadata"] = [] | ||
| elif augmentation_cls == A.CopyPaste: | ||
| data["copypaste_metadata"] = [] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if augmentation_cls == A.OverlayElements: | ||
| data = { | ||
| "image": image, | ||
| "overlay_metadata": [], | ||
| } | ||
| else: | ||
| data = { | ||
| "image": image, | ||
| } | ||
| data["overlay_metadata"] = [] | ||
| elif augmentation_cls == A.CopyPaste: | ||
| data["copypaste_metadata"] = [] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if augmentation_cls == A.OverlayElements: | ||
| data = { | ||
| "image": image, | ||
| "overlay_metadata": [], | ||
| } | ||
| else: | ||
| data = { | ||
| "image": image, | ||
| } | ||
| data["overlay_metadata"] = [] | ||
| elif augmentation_cls == A.CopyPaste: | ||
| data["copypaste_metadata"] = [] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if augmentation_cls == A.OverlayElements: | ||
| data = { | ||
| "image": image, | ||
| "overlay_metadata": [], | ||
| } | ||
| else: | ||
| data = { | ||
| "image": image, | ||
| } | ||
| data["overlay_metadata"] = [] | ||
| elif augmentation_cls == A.CopyPaste: | ||
| data["copypaste_metadata"] = [] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Fixes:
#745
#1716
#1784
#1676
#1297
#1225
#865
Summary by Sourcery
This pull request introduces a new
CopyPastetransformation for image augmentation, enhances theOverlayElementstransformation, and updates the test suite to cover the new functionality. Additionally, it includes a minor update to the pre-commit configuration.CopyPastetransformation for image augmentation, supporting image, mask, bounding boxes, and keypoints.OverlayElementstransformation to improve metadata handling and added an example usage in the docstring.apply_to_bboxes,apply_to_keypoints, andapply_to_masksmethods to useListinstead ofSequencefor type annotations.CopyPastetransformation to ensure correct functionality.CopyPastetransformation.OverlayElementsandCopyPastetransformations.