-
Notifications
You must be signed in to change notification settings - Fork 373
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
Show tensors shaped [H, W, 1, 1] as images (and more!) #2075
Conversation
… an image. Fixes #1871 Quite a bit of nuance to support single channel 1x1 images & line-like images.
10205f3
to
2efeb12
Compare
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.
Even more nuance required!
.enumerate() | ||
.rev() | ||
.find(|(_, dim)| dim.size != 1) | ||
.map_or(&self.shape[0..1], |(i, _)| &self.shape[..(i + 1)]) |
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'm starting to think we should have a struct Shape(Vec<TensorDimension>)
. It would also solve the formatting of shapes for which we currently use the ugly Debug
formatting in a bunch of places (one more in this PR).
Oh well, out of scope for this PR
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 added a comment about this to #1992
match shape_short.len() { | ||
1 => { |
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 shape_short
would return just the integers (not the names) we could make this match a lot nicer;
match shape_short.len() { | |
1 => { | |
match shape_short { | |
[h] => {…}, | |
[h, w] => {…}, | |
[h, w, c] => {…}, |
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.
…but it would require an allocation, or that we change how shape is stored (see #1992 (comment)), so let's not right now
(almost done addressing everything, but logging off now; finishing tomorrow) |
nuancing away! This one might warrant a short re-review. |
dim => self.image_height_width_channels().and_then(|_| { | ||
self.get( | ||
&[x, y, channel] | ||
.into_iter() | ||
.chain(std::iter::repeat(0).take(dim - 3)) | ||
.collect::<Vec<u64>>(), | ||
) | ||
}), |
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 think we should return None if using get_with_image_coords
on a tensor that is non-image like, e.g. 8x7x6x5x4x3
So maybe just check if self.short_shape().len() <= 3
in this branch
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.
the check in here implied by self.image_height_width_channels().and_then
is actually stronger and would also reject e.g. 3x3x5
So I think this is alright this way (albeit more expensive)
* Ignore trailing tensor dimension when determining whether a tensor is an image. Fixes #1871 Quite a bit of nuance to support single channel 1x1 images & line-like images. * fix image preview for images other than M x N x C and M x N * comment fix * better shape_short comment * handle empty tensors * is_shaped_like_an_image is now defined via image_height_width_channels, improve comment on both * any Nx1x... image now now treated as image * rename to get_with_image_coords, make it more restrictive * tensor_to_gpu height_width_depth utility uses now tensor.image_height_width_channels * change behavior of is_vector
Ignore trailing tensor dimension when determining whether a tensor is an image.
Fixes #1871
There's a surprising amount of nuance in order to support single channel 1x1 images & line-like images.
Checklist
PR Build Summary: https://build.rerun.io/pr/2075