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

Added the Ability to Use Rectilinear Polygons in the Dynamic Zone Block #654

Merged
merged 13 commits into from
Sep 20, 2024

Conversation

chandlersupple
Copy link
Contributor

Description

Added two optional parameters to the Dynamic Zone block allowing users to use rectangles instead of irregular polygons, and get the height, width, and angle of the rectangles.

dynamic-zone-block-pr

Type of change

  • New feature (non-breaking change which adds functionality)

How has this change been tested, please provide a testcase or example of how you tested the change?

Passed all tests. Added the test_calculate_minimum_bounding_rectangle test.

Any specific deployment considerations

N/A

Docs

N/A

@PawelPeczek-Roboflow
Copy link
Collaborator

@chandlersupple could you please follow up on @grzegorz-roboflow comments - otherwise it may not be pushed into next release

Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

I am approving this change, but I have one observation regarding interoperability of the block.

We claim the output is of kind list_of_values which is true, but include_rectangle_details changes the output data - making downstream blocks compatible or incompatible - depending on the value of this parameter. I am not sure how it ends up working - maybe that would be a problem, maybe not. @chandlersupple could you please monitor that over time and when we see people struggling with adoption of the block due to that inconsistency ping me to solve the problem.

@grzegorz-roboflow
Copy link
Contributor

I am approving this change, but I have one observation regarding interoperability of the block.

We claim the output is of kind list_of_values which is true, but include_rectangle_details changes the output data - making downstream blocks compatible or incompatible - depending on the value of this parameter. I am not sure how it ends up working - maybe that would be a problem, maybe not. @chandlersupple could you please monitor that over time and when we see people struggling with adoption of the block due to that inconsistency ping me to solve the problem.

@chandlersupple I moved all logic related to bounding rect into new block

Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

please add end-to-end test to integration tests and add to gallery

@hansent
Copy link
Contributor

hansent commented Sep 20, 2024

I agree this should be a separate block. Noticed that the output kind changed (was LIST_OF_VALUES via dynamic zone block, but new block has sv.detections as output); maybe that's what we want, but not sure what the desired use case is. @chandlersupple how / what is going to be using the rectangle data?

Main thing I can think of is difference between predictions and zones (for e.g. time in zone block), maybe we can have a block to convert between these.

@PawelPeczek-Roboflow PawelPeczek-Roboflow merged commit e22b900 into main Sep 20, 2024
58 checks passed
@PawelPeczek-Roboflow PawelPeczek-Roboflow deleted the regular-dynamic-zone branch September 20, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants