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

Rename bbox -> bounds #2199

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Rename bbox -> bounds #2199

merged 3 commits into from
Aug 6, 2024

Conversation

ashnair1
Copy link
Collaborator

@ashnair1 ashnair1 commented Jul 29, 2024

'bbox' in kornia augmentations is used to identify bounding boxes of instances in an image. However, in GeoDatasets, bbox refers to the geographic bounds of the data. This PR aims to make these distinct by renaming 'bbox' in GeoDatasets to 'geo_bbox'

bounds could also be another option.

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing datamodules PyTorch Lightning datamodules labels Jul 29, 2024
@adamjstewart adamjstewart added the backwards-incompatible Changes that are not backwards compatible label Jul 29, 2024
@adamjstewart
Copy link
Collaborator

adamjstewart commented Jul 29, 2024

I would also be okay with removing it entirely. We currently don't use it anywhere. The original intent was that it could be used for stitching together prediction patches, but we don't do that yet. But I like bounds better than geo_bbox.

@ashnair1 ashnair1 changed the title Rename bbox -> geo_bbox Rename bbox -> bounds Jul 29, 2024
@ashnair1 ashnair1 marked this pull request as ready for review July 29, 2024 18:10
@adamjstewart adamjstewart added this to the 0.6.0 milestone Jul 31, 2024
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Looks fine to me, hoping you can convince at least one more person to review the name choice so we don't end up changing it again later. Note that rtree supports both bounds and bbox for the same objects, but they have different meanings. This has always confused me.

@robmarkcole
Copy link
Contributor

I think bounds is fine, another option could be extent

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Aug 4, 2024

Looks fine to me, hoping you can convince at least one more person to review the name choice so we don't end up changing it again later. Note that rtree supports both bounds and bbox for the same objects, but they have different meanings. This has always confused me.

Looks like rtree's bbox is just an interleaved bounds. We could also consider query as another option.

@adamjstewart
Copy link
Collaborator

Based on popular vote, I think we'll stick with bounds.

@adamjstewart adamjstewart merged commit ddd16cb into microsoft:main Aug 6, 2024
19 checks passed
@ashnair1 ashnair1 deleted the bbox-rename branch August 6, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatible Changes that are not backwards compatible datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants