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.

Sign up for GitHub

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

Jump to bottom

[models] Change Resize kwargs to args for each zoo predictor #1765

Merged
merged 2 commits into from
Nov 4, 2024
Merged

[models] Change Resize kwargs to args for each zoo predictor #1765

merged 2 commits into from
Nov 4, 2024

Conversation

cmoscardi
Copy link
Contributor

Closes #1764 -- more details in that issue. Hope this all helps!

@cmoscardi cmoscardi force-pushed the fix_resize_inconsistency branch 2 times, most recently from 8c104b7 to b1a5e68 Compare October 29, 2024 00:49
@felixdittrich92 felixdittrich92 self-assigned this Oct 29, 2024
@felixdittrich92 felixdittrich92 added this to the 0.11.0 milestone Oct 29, 2024
@felixdittrich92 felixdittrich92 added type: enhancement Improvement module: models Related to doctr.models ext: tests Related to tests folder ext: references Related to references folder topic: text detection Related to the task of text detection labels Oct 29, 2024
felixdittrich92
felixdittrich92 requested changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

Hi @cmoscardi 👋,

Thanks for the PR 👍

I agree from user view it's cleaner but then we should keep it consistent over all _predictor instances and don't miss to update the docstrings :)

I created a branch as reference (feel free to copy from):
felixdittrich92@c62b851

@cmoscardi cmoscardi force-pushed the fix_resize_inconsistency branch 3 times, most recently from c7c9c30 to b3e84d6 Compare November 1, 2024 18:49
@felixdittrich92
Copy link
Contributor

@cmoscardi you can apply formatting with make style :)

@cmoscardi
Copy link
Contributor Author

@cmoscardi you can apply formatting with make style :)

Ah thank you! I am in a limited connectivity environment right now and will be able to get the dev dependencies installed in a few hours. I also wanted to run tests before pinging you, thanks for the quick review of this :)

@felixdittrich92
Copy link
Contributor

@cmoscardi you can apply formatting with make style :)

Ah thank you! I am in a limited connectivity environment right now and will be able to get the dev dependencies installed in a few hours. I also wanted to run tests before pinging you, thanks for the quick review of this :)

No stress 😁 👍

@codecov Codecov
Copy link

codecov bot commented Nov 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (d0eff67) to head (d96a092).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1765      +/-   ##
==========================================
+ Coverage   96.55%   96.60%   +0.05%     
==========================================
  Files         164      164              
  Lines        7895     7895              
==========================================
+ Hits         7623     7627       +4     
+ Misses        272      268       -4
Flag Coverage Δ
unittests 96.60% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

feat: Add more default params to predictors
684bfe6 
Specifically, batch_size, symmetric_pad, preserve_aspect_ratio.
This exposes a more consistent API for users across all predictors.

Resolves: mindee#1764
docs: fix quoting in CONTRIBUTING.md
d96a092 
Minor issue with pip install (on zsh at least, should be fine for other
shells too)
@cmoscardi
Copy link
Contributor Author

OK should be good now!

felixdittrich92
felixdittrich92 approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

@felixdittrich92 felixdittrich92 changed the title Fix resize inconsistency between detection_predictor() and ocr_predictor() default initialization [models] Change Resize kwargs to args for each zoo predictor Nov 4, 2024
@felixdittrich92 felixdittrich92 merged commit b411109 into mindee:main Nov 4, 2024
81 of 82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixdittrich92 felixdittrich92 felixdittrich92 approved these changes

Assignees

@felixdittrich92 felixdittrich92

Labels
ext: references Related to references folder ext: tests Related to tests folder module: models Related to doctr.models topic: text detection Related to the task of text detection type: enhancement Improvement
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.

Inconsistency between detection_predictor and ocr_predictor().det_predictor resize parameters
2 participants
@cmoscardi @felixdittrich92