Skip to content

Fix: syntax errors in utility function validations#1336

Open
abhayrajjais01 wants to merge 1 commit intoweecology:mainfrom
abhayrajjais01:fix-utility-warnings
Open

Fix: syntax errors in utility function validations#1336
abhayrajjais01 wants to merge 1 commit intoweecology:mainfrom
abhayrajjais01:fix-utility-warnings

Conversation

@abhayrajjais01
Copy link
Contributor

@abhayrajjais01 abhayrajjais01 commented Mar 6, 2026

Description

This PR addresses bugs in src/deepforest/utilities.py where validation checks would either crash the program or execute incorrect string length logic.

Changes:

  • Deprecation Warnings: Replaced raise DeprecationWarning(...) with warnings.warn(..., DeprecationWarning). The previous implementation threw an exception and killed the process, effectively making the functions unreachable.
  • String Length Check: Fixed a logic bug in __assign_image_path__ where len() was evaluated on the first character of the image path string instead of on the array of unique existing paths. This prevented false-positive warnings from firing on valid string paths.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
    nge which fixes an issue)

AI-Assisted Development

  • I used AI tools (e.g., GitHub Copilot, ChatGPT, etc.) in developing this PR
  • I understand all the code I'm submitting
  • I have reviewed and validated all AI-generated code

AI tools used (if applicable):

AI tools were initially used to understand the codebase better. but code was written and verified manually.

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.65%. Comparing base (efb872f) to head (f6427b8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/deepforest/utilities.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1336      +/-   ##
==========================================
+ Coverage   87.34%   87.65%   +0.31%     
==========================================
  Files          24       24              
  Lines        2978     2989      +11     
==========================================
+ Hits         2601     2620      +19     
+ Misses        377      369       -8     
Flag Coverage Δ
unittests 87.65% <91.66%> (+0.31%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

Hi @abhayrajjais01, thanks for the suggestions. Could you share the script you were running when you encountered this bug? If you discovered it using AI, that’s also okay. Once I have that information, I can better guide you on how to proceed.

@abhayrajjais01
Copy link
Contributor Author

Hi @henrykironde, thanks for reviewing!
Yes, I discovered these bugs through an AI, here is the context for each fix:

the original code used raise DeprecationWarning(...), which throws an exception and immediately terminates the function — making lines 231–236 unreachable dead code, any caller of shapefile_to_annotations() would always get a crash instead of the intended deprecation notice. the fix changes it to warnings.warn(..., DeprecationWarning) so the function actually completes and returns a result
In __assign_image_path__, the check len(existing_image_path) > 1 was called on a single path string, meaning the "multiple image paths" warning fired spuriously on virtually every call the fix changes the condition to check len(existing_image_paths) (the array of unique paths), which correctly warns only when there are actually multiple distinct image paths
let me know if you would like any other changes

@abhayrajjais01 abhayrajjais01 force-pushed the fix-utility-warnings branch 4 times, most recently from bfad71b to 2b7831a Compare March 7, 2026 13:40
@abhayrajjais01 abhayrajjais01 force-pushed the fix-utility-warnings branch 2 times, most recently from d07531b to f6427b8 Compare March 7, 2026 15:41
@henrykironde
Copy link
Contributor

If you take the time to fully understand the function, what it does, and how it is used, you may arrive at a good design or solution for this PR. I strongly encourage reviewing and understanding the code before submitting changes, rather than sending AI generated patches that may not yet be fully understood.

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

Please refer to the comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants