Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Add copy and paste #1676

wants to merge 2 commits into from

Conversation

ternaus
Copy link
Collaborator

@ternaus ternaus commented Apr 18, 2024

No description provided.

@ternaus ternaus marked this pull request as draft April 18, 2024 23:15
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link
Contributor

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)
Copy link
Contributor

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.

Suggested change
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:
Copy link
Contributor

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.

Suggested change
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": []}

Comment on lines +285 to +286
scale = 0
angle = 0
Copy link
Contributor

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)

Suggested change
scale = 0
angle = 0
scale = 0
angle = 0

@ternaus ternaus mentioned this pull request Jun 27, 2024
@ternaus ternaus closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant