-
Notifications
You must be signed in to change notification settings - Fork 825
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
feat: method to catch and classify overlapping bounding boxes #1803
Merged
Merged
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
174d983
feat: method to catch and classify overlapping
LaverdeS 1ff28cd
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS 37a1077
feat: catching 6 cases of overlapping bboxes
LaverdeS cd97282
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS ac406d2
chore: tidy
LaverdeS c7b36cc
chore: shorten catch_overlapping_and_nested_bboxes method
LaverdeS dac2e42
chore: typo in docstrings
LaverdeS ca07bf3
fix: max ngram n in calculate_largest_ngram_percentage
LaverdeS 85fd0fc
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS a580ab1
chore: add typing
LaverdeS d8c4f44
chore: add testing for case with and without overlap
LaverdeS b85225d
chore: update CHANGELOG
LaverdeS d95fa0b
chore: tidy
LaverdeS 78057ae
chore: better logic for is_parent_box
LaverdeS 7d69d9b
chore: tidier lines
LaverdeS 6c6317a
chore: tidy
LaverdeS d234fd3
chore: tidy
LaverdeS a700881
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS a74d158
chore: refactor ngrams() method
LaverdeS 26cf06c
chore: tidy
LaverdeS 32dcc41
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS c820900
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS 6f6eca7
feat: add nested_error_tolerance_px and sm_overlap_threshold params
LaverdeS 0dba67f
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS 861e496
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS 2abfcc1
chore: more tests to cover all cases and use params
LaverdeS 5c0c6d5
Merge branch 'main' into sebastian/catch_overlapping_bboxes
LaverdeS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is over 150 lines long, which is way over the rule of thumb limit of 5-20 lines. Breaking it up into logical subtasks that get their own functions would really increase the readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed
catch_overlapping_bboxes
tocatch_overlapping_and_nested_bboxes
and create two more methods to break it up:identify_overlapping_or_nesting_case
andidentify_overlapping_case
. A short description can be found in the PR description. Is it better? I can try to split it further but most of the conditions to classify the overlapping-case shall be in one method, making it large.