Skip to content

Conversation

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Aug 7, 2023

Towards #7803

This mostly just removes _wrap() from the base class.
Also minor cleanup of bbox code + some test.

cc @vfdev-5

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 7, 2023

🔗 Helpful Links

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

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

❌ 3 New Failures

As of commit 6237f0d:

NEW FAILURES - The following jobs have failed:

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

@NicolasHug NicolasHug changed the title Wrapppp Remove _wrap() class method from base class Datapoint Aug 7, 2023
Comment on lines +116 to +117
def test_no_wrapping_exceptions_with_metadata():
# Sanity checks for the ops in _NO_WRAPPING_EXCEPTIONS and datapoints with metadata
Copy link
Member Author

Choose a reason for hiding this comment

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

If you're wondering why I added this test, it's because I originally had changed the _NO_WRAPPING_EXCEPTIONS to return output.as_subclass(cls) instead of cls.wrap_like(input, output), and I didn't get a proper error in the tests. The only tests that were failing here were test_deepcopy() which was failing on print(bbox), which is a bit too "remote" of a test.

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green. Thanks!


@classmethod
def _wrap(cls, tensor: torch.Tensor, *, format: BoundingBoxFormat, canvas_size: Tuple[int, int]) -> BoundingBoxes: # type: ignore[override]
def _wrap(cls, tensor: torch.Tensor, *, format: Union[BoundingBoxFormat, str], canvas_size: Tuple[int, int]) -> BoundingBoxes: # type: ignore[override]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Datapoint._wrap is removed now, do we still need the mypy directive?

Suggested change
def _wrap(cls, tensor: torch.Tensor, *, format: Union[BoundingBoxFormat, str], canvas_size: Tuple[int, int]) -> BoundingBoxes: # type: ignore[override]
def _wrap(cls, tensor: torch.Tensor, *, format: Union[BoundingBoxFormat, str], canvas_size: Tuple[int, int]) -> BoundingBoxes:

@NicolasHug NicolasHug merged commit 9b82df4 into pytorch:main Aug 7, 2023
facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2023
Reviewed By: matteobettini

Differential Revision: D48642288

fbshipit-source-id: cce9803e2a4478afcbf56b052ff1ae39e1dc0b2d
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