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

temp commit #1580

Closed
wants to merge 1 commit into from
Closed

temp commit #1580

wants to merge 1 commit into from

Conversation

ternaus
Copy link
Collaborator

@ternaus ternaus commented Mar 13, 2024

No description provided.

@ternaus ternaus marked this pull request as draft March 13, 2024 01:59
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!

General suggestions:

  • Consider revising the error message in apply_to_mask to accurately reflect the unsupported operation.
  • Correct the typo in the dictionary key preprocessing_pipepline to preprocessing_pipeline to avoid potential runtime errors.
  • Evaluate the added complexity due to conditional imports and the introduction of new methods for managing external parameters. Consider if there are simpler alternatives that achieve the same goals.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Docstrings: all looks good

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment on lines +320 to +322
def apply_to_mask(self, mask: np.ndarray, mix_data: List[ReferenceImage], *args: Any, **params: Any) -> np.ndarray:
msg = "Mosaic does not support keypoints yet"
raise NotImplementedError(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (typo): The apply_to_mask method in the Mosaic class raises NotImplementedError with a message indicating that Mosaic does not support keypoints, which seems to be a copy-paste error from the apply_to_keypoints method. The error message should correctly reflect that it's the mask processing that is not implemented.

Suggested change
def apply_to_mask(self, mask: np.ndarray, mix_data: List[ReferenceImage], *args: Any, **params: Any) -> np.ndarray:
msg = "Mosaic does not support keypoints yet"
raise NotImplementedError(msg)
def apply_to_mask(self, mask: np.ndarray, mix_data: List[ReferenceImage], *args: Any, **params: Any) -> np.ndarray:
msg = "Mosaic does not support mask processing yet"
raise NotImplementedError(msg)

last_tile_height = last_tile[2] - last_tile[0]
preprocessing_pipeline = self.get_preprocessing_pipeline(last_tile_height, last_tile_width)

return {"mix_data": sampled_reference_data, "preprocessing_pipepline": preprocessing_pipeline}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (typo): There's a typo in the dictionary key preprocessing_pipepline; it should be preprocessing_pipeline. This typo could lead to runtime errors when accessing the returned dictionary values.

Suggested change
return {"mix_data": sampled_reference_data, "preprocessing_pipepline": preprocessing_pipeline}
return {"mix_data": sampled_reference_data, "preprocessing_pipeline": preprocessing_pipeline}

@@ -19,6 +19,10 @@
)
from .utils import format_args

if TYPE_CHECKING:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): The introduction of conditional imports for TYPE_CHECKING and the new update_with_external_params method increases the complexity of the class. The conditional imports, while a common pattern for type hints, add a conditional code path that's not executed at runtime, potentially making the code harder to follow for those unfamiliar with this pattern. Additionally, the update_with_external_params method, along with the management of external_bbox_params and external_keypoint_params, introduces more state into the class. This increases the complexity by adding more side effects and interactions to consider.

While these changes do introduce new functionality and flexibility, it's important to weigh the added complexity against the benefits. If the goal of these changes is to incorporate external parameters directly into the class, it might be challenging to simplify without compromising the new features. However, it's worth considering if there are alternative approaches that could achieve the same goals with less complexity, such as handling external parameters outside of the class to keep it focused on its primary responsibilities.

@ternaus ternaus closed this Oct 10, 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