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

remove spatial_size #7734

Merged
merged 6 commits into from
Aug 1, 2023
Merged

remove spatial_size #7734

merged 6 commits into from
Aug 1, 2023

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 11, 2023

The term "spatial size" was introduced together with v2 and is somewhat ambiguous. Its meaning is

  • .shape[-2:], i.e. height and width, for images, videos and masks
  • .shape[-2:], i.e. height and width, of the corresponding image, video, or mask for bounding boxes

TL;DR: This PR removes it.

  • Rename the BoundingBox.spatial_size attribute as well as all of the spatial_size parameters in our bounding box kernels, e.g.

    def horizontal_flip_bounding_box(
    bounding_box: torch.Tensor, format: datapoints.BoundingBoxFormat, spatial_size: Tuple[int, int]
    ) -> torch.Tensor:

    to canvas_size. That term is not perfect as well, but I feel it easier to interpret than spatial_size. Another candidate is reference_size. LMK if you prefer that.

  • Rename F.get_spatial_size and query_spatial_size to F.get_size and query_size. There is a semantic slip in that name in the sense that it still returns the canvas_size for bounding boxes. However, this is the same as before, where F.get_spatial_size returned the size for images, masks, and videos.

  • Remove the .spatial_size, .num_channels, and .num_frames properties from the image, mask, and video datapoint classes. They were exclusively used in the F.get_* functions. The functionality was moved there. They provided little convenience in the first place and keeping them public also clashes with make datapoint methods private #7733.

  • Revert part of the changes that extract make_* functions out of make_*_loader #7717 introduced to the testing function make_bounding_box. The function no longer takes a size as positional and a spatial_size as optional keyword argument, but only canvas_size as positional argument. Meaning, the role that size previously filled was removed, since we never used that.

cc @vfdev-5

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 11, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7734

Note: Links to docs will display an error until the docs builds have been completed.

❌ 23 New Failures

As of commit 276701f:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Philip, LGTM if green. Admittedly I didn't review every single line but the summary makes sense to me.

We still have a bunch of occurrences of "spatial_size" in the tests (e.g. in the utils). I assume these will be gone once we finish porting the V2 tests to their new version?

@pmeier pmeier merged commit 312c3d3 into pytorch:main Aug 1, 2023
37 of 60 checks passed
@pmeier pmeier deleted the canvas-size branch August 1, 2023 07:38
facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2023
Reviewed By: matteobettini

Differential Revision: D48642265

fbshipit-source-id: 123d2a3157d4536ea9ac25e0192d54307b31ea1e
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.

3 participants