-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor affine #2008
Refactor affine #2008
Conversation
Reviewer's Guide by SourceryThis pull request refactors the affine transformation functionality in the Albumentations library. The main changes include replacing the use of skimage's ProjectiveTransform with custom numpy-based affine transformation matrices, updating the rotation and other geometric transformations to use the new affine matrix approach, and adjusting various tests to accommodate these changes. Sequence diagram for applying affine transformation to an imagesequenceDiagram
participant Test
participant fgeometric
participant cv2
Test->>fgeometric: create_affine_transformation_matrix(...)
fgeometric-->>Test: affine_matrix
Test->>fgeometric: warp_affine(image, affine_matrix, ...)
fgeometric->>cv2: cv2.warpAffine(image, affine_matrix[:2], ...)
cv2-->>fgeometric: transformed_image
fgeometric-->>Test: transformed_image
Class diagram for updated affine transformation functionsclassDiagram
class fgeometric {
+create_affine_transformation_matrix(translate, shear, scale, rotate, shift) np.ndarray
+warp_affine(image, matrix, interpolation, cval, mode, output_shape) np.ndarray
+keypoints_affine(keypoints, matrix, image_shape, scale, mode) np.ndarray
+apply_affine_to_points(points, matrix) np.ndarray
+calculate_affine_transform_padding(matrix, image_shape) tuple[int, int, int, int]
+bboxes_affine_largest_box(bboxes, matrix) np.ndarray
+bboxes_affine_ellipse(bboxes, matrix) np.ndarray
}
note for fgeometric "Refactored to use numpy-based affine transformation matrices instead of skimage's ProjectiveTransform"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ternaus - I've reviewed your changes - here's some feedback:
Overall Comments:
- This refactoring of the affine transformation functionality looks like a solid improvement. The shift from skimage's ProjectiveTransform to NumPy arrays should enhance performance and reduce dependencies. However, given the extent of the changes, we recommend thorough testing with real-world use cases to ensure no regressions have been introduced.
- The removal of the rotate() function and consolidation into the general affine transformation framework may impact the API. Please ensure any API changes or deprecations are clearly documented and communicated to users.
- The additional test cases and attention to numerical stability are commendable. Consider adding more tests that cover a wide range of inputs and edge cases to verify the robustness of the new implementation.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Refactor affine transformation functions to use numpy arrays for matrix operations, replacing skimage dependencies. Enhance test coverage for affine transformations, including translation, scaling, rotation, shear, and combined transformations, to ensure correctness and robustness.
Enhancements:
Tests: