Skip to content

Conversation

sovrasov
Copy link
Member

@sovrasov sovrasov commented Jul 24, 2025

What does this PR do?

Adds support of representing non simply connected sets with contours: main contour describes the outer border of the set and "holes" are represented as child contours lying inside the space bounded by the outer one.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@github-actions github-actions bot added the python python related changes label Jul 24, 2025
@github-actions github-actions bot added tests Related to tests cpp C++ related changes build Related to build scripts labels Jul 25, 2025
@sovrasov sovrasov force-pushed the vs/support_nested_contours branch from ea673c9 to 8094ff5 Compare July 28, 2025 18:54
@sovrasov sovrasov marked this pull request as ready for review July 28, 2025 19:20
@sovrasov sovrasov requested a review from a team as a code owner July 28, 2025 19:20
ashwinvaidya17
ashwinvaidya17 previously approved these changes Jul 29, 2025
self.shape = shape
self.label = label
self.probability = probability
self.excluded_shapes = excluded_shapes
Copy link

Choose a reason for hiding this comment

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

For self.shape and self.excluded_shapes, I would suggest to force the type to be np.ndarray or list[tuple[int, int]], but not both. Otherwise clients who consume contour.shape have to support both types, which isn't great.

Note: this is different from the class constructor parameter, which is instead an input interface. There, supporting more types is helpful for the clients. This principle is somehow formulated in the Postel law.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally prefer garbage in garbage out principle: the system should not help clients if they don't understand that they're doing. Looks like to much hustle for a wrong type annotation, which we can not just drop. Added dummy conversion to numpy.

Copy link

@leoll2 leoll2 Jul 29, 2025

Choose a reason for hiding this comment

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

Thanks for fixing it!

the system should not help clients if they don't understand that they're doing

I don't fully understand this part; the type hints come from the library and help the client build robust code through static type checking (eg mypy). If the types are imprecise or loose, the client is forced to implement extra logic or suppress the warnings.

@sovrasov sovrasov merged commit c24d659 into open-edge-platform:master Jul 29, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Related to build scripts cpp C++ related changes python python related changes tests Related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants