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

fix: InferenceSlicer overlap_ratio_wh argument changed to None by default #1547

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

tibeoh
Copy link
Contributor

@tibeoh tibeoh commented Sep 26, 2024

Description

Issue #1543

Changed the default value of the overlap_ratio_wh argument to None, as this parameter will be deprecated in version 0.27.0. This change allows a smoother transition by not forcing users to explicitly set it to None.

⚠️ However, this may impact users who were relying on the default value of overlap_ratio_wh being (0.2, 0.2). They will now need to explicitly specify this value if they still wish to use it, as the default is now set to None.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

I have initiated a new test suite for this class (this is a first step). The tests cover validation of the overlap arguments (overlap_wh and overlap_ratio_wh) to ensure proper error handling and correct behavior. Additionally, initial tests for the _generate_offset method have been added to verify that slices are generated correctly based on the provided overlap settings.

Any specific deployment considerations

N/A

Docs

  • Docs updated? What were the changes: Default argument overlap_ratio_wh in InferenceSlicer class

…ault

Changed the default value of the overlap_ratio_wh argument to None, as this parameter will be deprecated in version 0.27.0. This change allows a smoother transition by not forcing users to explicitly set it to None.

Additionally, added tests for validating the overlap arguments (overlap_wh and overlap_ratio_wh) to ensure proper error handling and usage. Also started implementing tests for the offset generation method to verify that slices are correctly calculated based on the overlap settings.
@CLAassistant
Copy link

CLAassistant commented Sep 26, 2024

CLA assistant check
All committers have signed the CLA.

@LinasKo
Copy link
Contributor

LinasKo commented Sep 26, 2024

Hi @tibeoh,

It's very refreshing to see a thorough test suite together with the changes. Thank you for that!

I'd like to ask for a rollback of the deprecated parameter to (0.2, 0.2). Yes, changing it to None makes the transition more seamless for new users, but keeping support of old values is non-negotiable. Who knows - maybe this is keeping a manufacturing line operational somewhere in the world.

Yes, this makes the deprecation a bit awkward. When the day comes, users will have an explicit overlap_ratio_wh=None argument in their code. That is a problem for another day, but we may declare a secondary deprecation stage, where we allow overlap_ratio_wh=None, but warn the users that it does nothing and should be removed.

@LinasKo
Copy link
Contributor

LinasKo commented Sep 26, 2024

Also, would you mind creating a usage example in a Colab? You may use our Starter Template. This eases our review considerably and creates a way for us to look back at how features worked in the past.

@tibeoh
Copy link
Contributor Author

tibeoh commented Sep 26, 2024

Hi @LinasKo,

Thank you for the feedback.

I completely agree with rolling back the default value of overlap_ratio_wh to (0.2, 0.2), and I will make that change shortly.

However, I currently don't have the time to work on the Colab example. Is it possible to proceed with the PR without the Colab for now, or would it be better to address it later?

Additionally, besides reverting the default value and the tests I’ve already added, do you expect any further changes? For example, I noticed that the current documentation could be a bit contradictory. Would you like me to edit that as well?

@LinasKo
Copy link
Contributor

LinasKo commented Sep 26, 2024

This time - sure, let's skip the Colab. The change is small, and I can do a few tests on my end instead.

@LinasKo
Copy link
Contributor

LinasKo commented Sep 26, 2024

If there's contradictions in the docs, feel free to address that! We'll accept all the help we can get.

Alternatively, if you lack time, let me know where the issues lie, and I'll work on them when I have time.

@LinasKo
Copy link
Contributor

LinasKo commented Oct 1, 2024

Hi @tibeoh 👋

We'd love to have this fix in the new release - there are a few outstanding changes, but I'll take over and get it merged.

Thank you for your contribution!

* We want users to set it to None manually, and the decorator would produce a warning every time.
@LinasKo LinasKo merged commit 52c9ec0 into roboflow:develop Oct 1, 2024
9 checks passed
@tibeoh
Copy link
Contributor Author

tibeoh commented Oct 3, 2024

Hi @LinasKo
I was very busy, so thank you for finalizing my PR. I’ll try to submit new PRs in the future if I have any proposals for this library.

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.

3 participants