-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix ElasticTransform blur kernel and add missing args #2378
Conversation
Reviewer's Guide by SourceryThis pull request fixes the ElasticTransform blur kernel, corrects the Sequence diagram for generate_displacement_fields with blursequenceDiagram
participant fgeometric.generate_displacement_fields
participant cv2.GaussianBlur
fgeometric.generate_displacement_fields->>cv2.GaussianBlur: Apply Gaussian blur to displacement fields
activate cv2.GaussianBlur
cv2.GaussianBlur-->>fgeometric.generate_displacement_fields: Blurred displacement fields
deactivate cv2.GaussianBlur
fgeometric.generate_displacement_fields->>fgeometric.generate_displacement_fields: Scale by alpha
Updated class diagram for ElasticTransformclassDiagram
class ElasticTransform {
-alpha: float
-sigma: float
-approximate: bool
-same_dxdy: bool
-noise_distribution: str
-keypoint_remapping_method: str
-border_mode: int
-fill: tuple[float, ...] | float
-fill_mask: tuple[float, ...] | float
+__init__(
alpha: float,
sigma: float,
approximate: bool,
same_dxdy: bool,
interpolation: int,
mask_interpolation: int,
noise_distribution: str,
keypoint_remapping_method: str,
border_mode: int,
fill: tuple[float, ...] | float,
fill_mask: tuple[float, ...] | float,
p: float
)
+get_params_dependent_on_data(data: dict[str, Any]) : dict[str, Any]
+get_transform_init_args_names() : tuple[str, ...]
}
ElasticTransform --|> BaseDistortion
note for ElasticTransform "Added border_mode, fill, and fill_mask attributes and constructor parameters."
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 @liamjwang - I've reviewed your changes - here's some feedback:
Overall Comments:
- It might be worth adding a test case that specifically checks the behavior of
ElasticTransform
with the newborder_mode
,fill
, andfill_mask
parameters.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
Do not really have material effect in Elastic Transform |
Thank you! |
@liamjwang you still need to update the code for all checks to pass. Please read Contributoru's guide - https://github.com/albumentations-team/albumentations/blob/main/CONTRIBUTING.md |
Hello, a few suggested changes to ElasticTransform.
I think the
approximate
argument was reversed by mistake.Also, currently the blur step is skipped if the kernel size is (0, 0).
However, I think it was intended to let
cv2.GaussianBlur
automatically compute the kernel size.With this change, the behavior matches the ElasticTransform output from older versions (e.g. 1.4.15).
Also added
border_mode
,fill
, andfill_mask
args.Summary by Sourcery
Fixes issues in the ElasticTransform augmentation, including a reversed
approximate
argument, skipping the blur step when the kernel size is (0, 0), and missingborder_mode
,fill
, andfill_mask
arguments.Bug Fixes:
approximate
argument was reversed in ElasticTransform.Enhancements:
border_mode
,fill
, andfill_mask
arguments to ElasticTransform to provide more control over the transformation.