Close #511, replaced convert_to_boxes with geometry_type #514
Conversation
There was a problem hiding this comment.
This looks really good @JSpencerPittman! Thanks for the contribution!
Other than a wording suggestion (included in-line) I have two suggestions:
- Let's add some argument checking code since
geometry_typeis now a string instead of a boolean. Given the structure of the code the easiest thing is probably an assert statement at the beginning on the function that checks thatgeometry_typeis eitherboundingboxorpointand throws an error informing that user that it needs to be one of those two if something else is passed. - My one other thought (and I realize I'm suggesting we consider a change to my own wording from convert_to_boxes arg name in shapefile_to_annotations is poorly named #511), is to mirror pytorch's typical naming for "bounding boxes". A quick search suggest that "box"/"boxes" and "bbox" are both commonly used, so let's pick one of those for better consistency.
|
Just finished pushing the changes. The geometry_type should be more secure and understandable now. |
There was a problem hiding this comment.
Looks fantastic @JSpencerPittman! We've got a style test failing. You can fix it by running:
yapf -i --recursive deepforest/ --style=.style.yapf
or manually making the change indicted in the tests:
- raise ValueError(f"Invalid argument for 'geometry_type'. Expected 'point' or 'bbox'.")
+ raise ValueError(
+ f"Invalid argument for 'geometry_type'. Expected 'point' or 'bbox'.")
Since we need one more change anyway I've also noted one last docstring style change and then we can get this in.
Thanks for your work on this!
deepforest/utilities.py
Outdated
| Args: | ||
| shapefile: Path to a shapefile on disk. If a label column is present, it will be used, else all labels are assumed to be "Tree" | ||
| rgb: Path to the RGB image on disk | ||
| savedir: Directory to save csv files | ||
| buffer_size: size of point to box expansion in map units of the target object, meters for projected data, pixels for unprojected data. The buffer_size is added to each side of the x,y point to create the box. | ||
| convert_to_boxes (False): If True, convert the point objects in the shapefile into bounding boxes with size 'buffer_size'. | ||
| geometry_type (bbox): Specifies the spatial representation used in the shapefile; can be "bbox" or "point" |
There was a problem hiding this comment.
Let's drop the (bbox) here for consistency. We should probably do a general docstring consistency cleanup here, but the old (False) isn't consistent with any of the major styles so let's just drop it here since we're changing it.
|
Alright, it should pass the style test now. I'm very new to Open Source Contributing. So, thank you for the help. |
|
Thanks @JSpencerPittman! We're always happy to help out new contributors. |
This is just a simple swap with convert_to_boxes to with geometry_type to allow polygons to be used in the shape file further down the road. I also added some documentation regarding the handling of different geometries within the shape-file in the doc-strings body.