-
Notifications
You must be signed in to change notification settings - Fork 7k
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
add gallery example for datapoints #7321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Philip, looks good.
Just few comments to improve the content
gallery/plot_datapoints.py
Outdated
# | ||
# Each datapoint class takes any tensor-like data that can be turned into a :class:`~torch.Tensor` | ||
|
||
image = datapoints.Image([[0, 1], [1, 0]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid giving examples with 2d images.
image = datapoints.Image([[0, 1], [1, 0]]) | |
image = datapoints.Image(torch.randint(0, 256, torch.randint(size=(3, 32, 32))) |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for not using 2D, but since we print below, I don't want to use (3, 32, 32). I'll just add another dimension, so that we get (1, 2, 2)
. LMK if that is ok with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can maybe just print the shape or few values like image[0, :5, :5]
instead of everything ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we also have to fix the random seed. Plus later in the example, we perform an addition and multiplication on the image and thus having simple values like 0 and 1 is easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that showing examples with untypical data like 2d images or single channel image as (1, 2, 2) can lead to a certain confusion, like "why they do not use typical examples but some toy stuff". This is how I would see it.
If you would like to have RGB deterministic image, you can do then torch.arange(3 * 32 * 32).view(3, 32, 32)
To show the result of inplace addition and multiplication you can just fetch single pixel value: image[0, 1, 2]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you would like to have RGB deterministic image, you can do then
torch.arange(3 * 32 * 32).view(3, 32, 32)
Well that creates int64 data, which is also not terribly realistic, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK OK :) you know how to make it uint8
, right ? Anyway, I'm not forcing you to change, this is my opinion. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've played around with it, and is ok albeit more verbose. However, it somewhat bypasses the point in the comment above the line:
Each datapoint class takes any tensor-like data that can be turned into a :class:
~torch.Tensor
This makes little sense when we just pass a tensor into it as example.
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
…into datapoints-gallery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Philip
# corresponding image alongside the actual values: | ||
|
||
bounding_box = datapoints.BoundingBox( | ||
[17, 16, 344, 495], format=datapoints.BoundingBoxFormat.XYXY, spatial_size=image.shape[-2:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but personally, I find str usage to be much nicer and I feel like we should encourage it (feel free to disagree)
[17, 16, 344, 495], format=datapoints.BoundingBoxFormat.XYXY, spatial_size=image.shape[-2:] | |
[17, 16, 344, 495], format="XYXY", spatial_size=image.shape[-2:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a more general discussion. Before you joined, the consensus was we use the enums for everything and only silently support strings at all. This is why none of the kernels even support strings. Thus, I feel we should be consistent and use the enum everywhere.
I personally like the enum better than string, but we can have an open discussion about that. However, if we decide str should be the "default" there is no need for the enum at all.
Hey @pmeier! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Co-authored-by: vfdev <vfdev.5@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Summary: Reviewed By: vmoens Differential Revision: D44416576 fbshipit-source-id: 6c44f22b2e15a66bc0f9cba39ff55439c2dd21e9 Co-authored-by: vfdev <vfdev.5@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
No description provided.