Skip to content

Polygonzone add multiple anchors support - #910

Merged
onuralpszr merged 13 commits into
roboflow:developfrom
LeviVasconcelos:polygonzone-add-multiple-anchors-support
Feb 28, 2024
Merged

Polygonzone add multiple anchors support#910
onuralpszr merged 13 commits into
roboflow:developfrom
LeviVasconcelos:polygonzone-add-multiple-anchors-support

Conversation

@LeviVasconcelos

@LeviVasconcelos LeviVasconcelos commented Feb 15, 2024

Copy link
Copy Markdown
Contributor

Description

This PR addresses Issue #844.

  • Rename triggering_position to triggering_anchors to be consistent with the LineZone naming convention.
  • Update type of argument from Position to Iterable[Position].
  • Maintain the default behavior. The zone should, by default, be triggered by Position.BOTTOM_CENTER.
  • Adds unit tests for PolygonZone.
  • Updates documentation.

Type of change

  • [ x ] New feature (non-breaking change which adds functionality)
  • [ x ] This change requires a documentation update

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

For a minimalist setup, I'm doing the following:

1- Create mock detections simulating an object moving in a straight line.
2- Instantiate a PolygonZone object with polygon partially intersecting the line.
3- Compute the PolygonZone.trigger(mock_detections) and annotate the detections triggering the PolygonZone object as green, and red otherwise.

The setup used here was also used as base for the unit tests.

Google colab: https://colab.research.google.com/drive/1fcsVrprMuf7hm_bxitSqmeL5L7KBVGHh?usp=sharing

Deployment Concerns

API Backward Compatibility

Option 1: Maintaining Deprecation Logic Within the Function

To ensure backward compatibility after renaming a parameter in this PR, one approach is to maintain deprecation logic within the function itself. The implementation could look something like this:

class PolygonZone:
    def __init__(self, polygon, frame_resolution_wh, triggering_anchors, **kwargs):
        if 'triggering_position' in kwargs:
            warning.warn('deprecated argument...')
            ''' logic to handle deprecated parameter'''

While effective for specific cases, this approach has some drawbacks:

  1. Documentation Readability: Inserting **kwargs might make the documentation less clear.
  2. Code Visibility: Deprecated code may become challenging to locate, especially with scale.
  3. Lack of Precedence: I couldn't find any examples of this pattern in the codebase.

Option 2: Adding a @deprecated_parameter Annotator (Adopted Solution)

The chosen solution involves creating a @deprecated_parameter annotator using pseudocode like this:

def deprecated_parameter(old_param: str, new_param: str, map_func: Callable = lambda x: x):
    def decorator(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            if old_param in kwargs:
                print('warning message...')
                kwargs[new_param] = map_func(kwargs.pop(old_param))
            return func(*args, **kwargs)
        return wrapper
    return decorator

While this solution may not be suitable for highly complex parameter changes, it aligns with existing practices, such as the use of the @deprecated annotator, making it easier to track deprecated code references.

Please share your thoughts on this proposed solution.

Deprecated Examples

Some examples, like examples/traffic_analysis, explicitly set triggering_position for the PolygonZone class. This PR renders it deprecated.

Should we address these changes within this PR, or would it be more appropriate to handle them in a separate one? I am open to incorporating the necessary modifications here if that is the preferred approach.

Let me know your preferences regarding these considerations.

Docs

  • [ x ] Docs updated? What were the changes:

Changed the documentation for class PolygonZone.

Fix : #844

@CLAassistant

CLAassistant commented Feb 15, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@LeviVasconcelos
LeviVasconcelos force-pushed the polygonzone-add-multiple-anchors-support branch from 617a445 to b5452f6 Compare February 15, 2024 21:34
Comment thread supervision/utils/internal.py Outdated
Comment thread test/detection/test_polygonzone.py Outdated
Comment thread test/detection/test_polygonzone.py Outdated
Comment thread supervision/detection/tools/polygon_zone.py
@SkalskiP

Copy link
Copy Markdown
Collaborator

Hi @LeviVasconcelos 👋🏻 ! Awesome PR. I liked your ideas. I left some comments. I'd appreciate it if you consider my feedback.

@LeviVasconcelos
LeviVasconcelos force-pushed the polygonzone-add-multiple-anchors-support branch from b01950c to 8b443d1 Compare February 26, 2024 20:42
@LeviVasconcelos

LeviVasconcelos commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for your comments, @SkalskiP ! I've addressed all of them.

Please let me know what you think

Comment thread supervision/detection/tools/polygon_zone.py
Comment thread supervision/utils/internal.py Outdated
Comment thread supervision/detection/tools/polygon_zone.py
@onuralpszr
onuralpszr self-requested a review February 28, 2024 17:27
@onuralpszr
onuralpszr force-pushed the polygonzone-add-multiple-anchors-support branch from 01c0281 to 7adb368 Compare February 28, 2024 19:04
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@onuralpszr
onuralpszr force-pushed the polygonzone-add-multiple-anchors-support branch from 7adb368 to d0ad5fe Compare February 28, 2024 19:10
@onuralpszr

Copy link
Copy Markdown
Contributor

@LeviVasconcelos thank you for your contribution I am merging in. For examples I did not touch it. There is little bit more work I see in example let's do them outside.

@onuralpszr
onuralpszr requested a review from SkalskiP February 28, 2024 19:22
@onuralpszr
onuralpszr dismissed SkalskiP’s stale review February 28, 2024 19:23

I fixed all changes as has been requested.

@onuralpszr
onuralpszr merged commit a790040 into roboflow:develop Feb 28, 2024
@LeviVasconcelos

Copy link
Copy Markdown
Contributor Author

Thank you guys @SkalskiP @onuralpszr.

It was a big pleasure to contribute!

Congratulation for the awesome project you guys are building :) !

@SkalskiP

Copy link
Copy Markdown
Collaborator

Thanks for your help @LeviVasconcelos 👋🏻 ! I really like your deprecated_parameter. I plan to use it a lot!

@onuralpszr onuralpszr added this to the version: 0.19.0 milestone Feb 29, 2024
@onuralpszr onuralpszr added API:polygonzone PolygonZone API enhancement New feature or request python Pull requests that update Python code labels Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API:polygonzone PolygonZone API enhancement New feature or request python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PolygonZone] - allow triggering_position to be Iterable[Position]

4 participants