-
-
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
Add copy and paste #1676
Add copy and paste #1676
Conversation
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 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
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
angle = 0 | ||
result.append((x + x_translation, y + y_translation, angle, scale, label)) | ||
|
||
return keypoints |
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.
issue (bug_risk): Return statement might not reflect intended changes to keypoints.
The method modifies and appends to the 'result' list but returns the original 'keypoints' list. Consider returning 'result' instead to reflect the changes made within the method.
or (start_y + target_image.shape[0] > original_image.shape[0]) | ||
): | ||
msg = "Target image and mask must fit within the dimensions of the original image" | ||
raise ValueError(msg) |
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.
suggestion (code_clarification): Consider adding more specific error handling for dimension validation.
The current error message is generic. Providing specific details about which dimension constraint was violated can help in debugging and using the function correctly.
raise ValueError(msg) | |
msg = ( | |
"Target image and mask must fit within the dimensions of the original image. " | |
"Required: (start_x + target_image.shape[1] <= original_image.shape[1]) and " | |
"(start_y + target_image.shape[0] <= original_image.shape[0])." | |
) | |
raise ValueError(msg) |
return {"mix_data": [], "embedding_coordinates": []} | ||
|
||
# If mix_data is None or empty after the above checks, return default values | ||
if reference_element is None: |
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.
suggestion (code_clarification): Consider handling the case where 'reference_element' is None more explicitly.
It's good to return default values when 'reference_element' is None, but consider logging this event or handling it in a way that's traceable for easier debugging and maintenance.
if reference_element is None: | |
import logging | |
logger = logging.getLogger(__name__) | |
if reference_element is None: | |
logger.debug("reference_element is None, returning default values.") | |
return {"mix_data": [], "embedding_coordinates": []} |
scale = 0 | ||
angle = 0 |
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.
suggestion (code-quality): Hoist statements out of for/while loops (hoist-statement-from-loop
)
scale = 0 | |
angle = 0 | |
scale = 0 | |
angle = 0 |
No description provided.