-
Notifications
You must be signed in to change notification settings - Fork 231
Fix read_file for shapefiles without image_path/label columns (#997) #1242
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
base: main
Are you sure you want to change the base?
Fix read_file for shapefiles without image_path/label columns (#997) #1242
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1242 +/- ##
==========================================
+ Coverage 87.73% 87.79% +0.05%
==========================================
Files 20 20
Lines 2716 2770 +54
==========================================
+ Hits 2383 2432 +49
- Misses 333 338 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the contribution. A couple of comments on the tests, there is quite a lot of duplicate code and perhaps it would be better to use pytest fixtures for some of the input data? Please could you also include the AI assistance declaration from the PR template? (you can see an example here) |
|
@jveitchmichaelis Thanks for the feedback! I've addressed both points:
|
henrykironde
left a comment
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.
@SuMayaBee Thanks for your perspective on the issue. The function you’re modifying has changed; could you rebase and update this PR so we can confirm whether these changes are still needed?
622f1d7 to
ea257ed
Compare
ea257ed to
e45b597
Compare
|
@henrykironde Thanks! I've rebased onto upstream/main and updated the implementation to work with the new function structure. The changes are still needed - I've integrated them and fixed a bug where label override wasn't being applied. All tests pass. Ready for review! |
Description
Allow users to pass
image_pathandlabelarguments toread_file()when reading shapefiles that don't have these columns.Problem
When users have a shapefile without
image_pathorlabelcolumns,read_file()crashes with:Solution
Pass
image_pathargument toshapefile_to_annotations()- Modifiedutilities.pyto forward theimage_pathargument when reading shapefiles.Add warning when
image_pathis passed - Users see a warning confirming the value will be assigned to every row.Allow
labelargument - Removed blocking error for missing label column. Now defaults to "Unknown" if not provided.Write tests for shapefiles - Added 4 tests covering all scenarios.
Add documentation example - Added example in
docs/user_guide/01_Reading_data.mdwith argument table showing required vs optional parameters.Before & After
For shapefiles without
image_pathandlabelcolumns:image_pathargumentValueErrorimage_pathandlabelargumentsValueErrorFor shapefiles with
image_pathandlabelcolumns:image_pathargumentlabelargumentUsage Improvement
Previously (workaround):
This required two extra library imports (
osandgeopandas).Now:
No extra library imports needed. Here,
root_diris optional whenimage_pathis a full path.labelis optional too and defaults to "Unknown" when not provided.Related Issue(s)
Closes #997
AI-Assisted Development