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

Modified boxes_to_shapefile function #597

Closed
wants to merge 2 commits into from

Conversation

Om-Doiphode
Copy link
Contributor

Fixes Issue #516:

The following modifications have been made:

  • Updated the boxes_to_shapefile function in utilities.py to use the 'rgb' argument instead of 'image_path'.
  • Modified the test functions in test_utilities.py to ensure the correct functionality of the modified boxes_to_shapefile function.

@ethanwhite ethanwhite requested a review from bw4sz March 21, 2024 12:47
@bw4sz
Copy link
Collaborator

bw4sz commented Apr 1, 2024

We just need to update docs and examples to match. Quick search found

https://deepforest.readthedocs.io/en/latest/annotation.html#how-can-i-view-current-predictions-as-shapefiles

@ethanwhite
Copy link
Member

I like this change, but note that it is backwards breaking (including for some of the lab's code). Ideally once we merge this the next release should be 2.0. A couple of possible ways to avoid needing to do this:

  1. One option when deprecating positional args is to replace the function with one with a new name and then deprecate the old function. This might actually be the best solution here since then we wouldn't be rushed into 2.0. Possible names include reproject_boxes, boxes_to_projection, boxes_to_original_projection.
  2. This also relates to Understanding the issue #608 #638 and Outputs should attach geometry if available #608. So one option is to move the officially supported behavior to inside the relevant functions with an option argument to maintain projection when present. We then deprecate the function instructing the user to use the optional argument to the relevant function (and since this is now internal we avoid future issues with user facing changes).
  3. If (1) and (2) don't work for some reason then we could at least check the new rgb argument to see if it includes a filename. If it doesn't then perform the previous behavior and throw a deprecation warning. This would at least prevent breaking code where root_dir was used positionally.

Thoughts?

Copy link
Member

@ethanwhite ethanwhite left a comment

Choose a reason for hiding this comment

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

See my comment about backwards compatibility and alternative approaches before merging this.

@ethanwhite
Copy link
Member

@bw4sz - can you response to #597 (comment) when you have a minute so that we can get this one moving forward

@bw4sz
Copy link
Collaborator

bw4sz commented May 13, 2024

I don't think we are ready for this change. There is a geo-type branch that will hold points, polygons, and boxes as part of geodataframe objects. We should wait until I finish that branch this summer.

#590

@ethanwhite
Copy link
Member

OK. Thanks @bw4sz. Given that I'm going to close this PR and I've assigned #516 to you so that folks know not to work on it further until you say so. I'm going to move the text of my comment over there.

@ethanwhite ethanwhite closed this May 15, 2024
@ethanwhite
Copy link
Member

Thanks for the contribution @Om-Doiphode - we just weren't quite ready for this one yet. Once @bw4sz lets us know this is ready to proceed we can chat about coming back to it.

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