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

More datapoints docs and comments #7830

Merged
merged 5 commits into from
Aug 15, 2023
Merged

Conversation

NicolasHug
Copy link
Member

No description provided.

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 15, 2023

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Just few comments otherwise looks good to me. Thanks!

gallery/plot_datapoints.py Outdated Show resolved Hide resolved
#
# You can re-wrap a pure tensor into a datapoint by just calling the datapoint
# constructor, or by using the ``.wrap_like()`` class method (see more details
# above in :ref:`datapoint_creation`):

new_bboxes = bboxes + 3
new_bboxes = datapoints.BoundingBoxes.wrap_like(bboxes, new_bboxes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

non-blocking thought to reduce the number of chars to type, it could be simple to have bboxes.wrap_like(new_bboxes) or bboxes.wrap_to(new_bboxes).
I think we already thought about that earlier but failed to get a good naming ...

Copy link
Member Author

@NicolasHug NicolasHug Aug 15, 2023

Choose a reason for hiding this comment

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

Yeah, I find it a bit long and clunky as well.

bboxes.wrap_to(new_bboxes).

That would be the best UX but unfortunately we can't do that because wrap_to would have to be a method on pure Tensors.

We can implement instance methods on BBoxes objects, but I can't think of a good name either right now

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have #7780 to go from BoundingBoxes to BBoxes. I'm somewhat against doing bboxes.wrap_like(...) here, since it no longer makes it clear that this is a class and not instance method.

gallery/plot_datapoints.py Outdated Show resolved Hide resolved
gallery/plot_datapoints.py Outdated Show resolved Hide resolved
gallery/plot_datapoints.py Outdated Show resolved Hide resolved
torchvision/datapoints/_datapoint.py Outdated Show resolved Hide resolved
#
# You can re-wrap a pure tensor into a datapoint by just calling the datapoint
# constructor, or by using the ``.wrap_like()`` class method (see more details
# above in :ref:`datapoint_creation`):

new_bboxes = bboxes + 3
new_bboxes = datapoints.BoundingBoxes.wrap_like(bboxes, new_bboxes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have #7780 to go from BoundingBoxes to BBoxes. I'm somewhat against doing bboxes.wrap_like(...) here, since it no longer makes it clear that this is a class and not instance method.

@NicolasHug NicolasHug merged commit f4f685d into pytorch:main Aug 15, 2023
45 of 62 checks passed
facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2023
Summary: Co-authored-by: vfdev <vfdev.5@gmail.com>

Reviewed By: matteobettini

Differential Revision: D48642271

fbshipit-source-id: 8982f6b16044a1f0877db664fc099b74049dcdff
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.

4 participants