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

BoundingBox: add area/volume attributes #375

Merged
merged 3 commits into from
Feb 10, 2022
Merged

Conversation

adamjstewart
Copy link
Collaborator

@adamjstewart adamjstewart commented Jan 28, 2022

This PR adds area/volume attributes to the BoundingBox class.

This is needed for a check I would like to add for #319, but I figured I would submit it as a separate PR to keep things easier to review.

@adamjstewart adamjstewart added this to the 0.3.0 milestone Jan 28, 2022
@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing labels Jan 28, 2022
@calebrob6
Copy link
Member

It feels weird multiplying area by time and having "volume" in a "Bounding Box", what is the bigger picture?

@adamjstewart
Copy link
Collaborator Author

Bounding boxes in TorchGeo have always been 3D boxes, I think volume makes sense here. We could also add an area attribute that only computes the spatial area.

The context is that I'm planning on adding something like this to IntersectionDataset._merge_dataset_indices:

box1 = BoundingBox(*hit1.bounds)
box2 = BoundingBox(*hit2.bounds)
box3 = box1 & box2
if box3.volume > 0 or box1.volume * box2.volume == 0:
    self.index.insert(i, tuple(box3))
    i += 1

This is designed to help with #319. The idea is that an intersection dataset should only contain bounding boxes with zero volume if the original bounds also have zero volume (point data). This is for the case where we have two side-by-side bboxes and end up with 2 additional areas of intersection with 0 overlap. It might make more sense to use area though since maxt - mint is often 0.

@calebrob6
Copy link
Member

Cool makes sense to me!

This would provide the area attribute for #380 as well.

@adamjstewart
Copy link
Collaborator Author

It might make more sense to use area though since maxt - mint is often 0.

Actually, this isn't true. disambiguate_timestamp resolves files to the microsecond resolution, but files rarely record the microsecond, so maxt - mint is rarely 0. But also, it's rare for timestamps to overlap. Even for sequential data like annual CDL, we subtract 1 microsecond from the end of the year, so there shouldn't be any overlap. So basically, we can use either area or volume, it shouldn't make a huge difference either way.

@adamjstewart adamjstewart changed the title BoundingBox: add volume attribute BoundingBox: add area/volume attributes Feb 8, 2022
@adamjstewart
Copy link
Collaborator Author

Added an area attribute and clarified what we mean by "area" and "volume" in the docstrings. This should be good to go now.

@adamjstewart adamjstewart merged commit 9e0e30a into main Feb 10, 2022
@adamjstewart adamjstewart deleted the features/bbox-volume branch February 10, 2022 22:53
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* BoundingBox: add volume attribute

* Style fixes

* BoundingBox: add area attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants