Expand the channels to 3 if the user requested as such #8229
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8229
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit e8979fc with merge base 7f55a1b ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Thanks a lot for the PR @ahmadsharif1 ! I made some minor comments below, but I'll approve now to unblock.
The tests are properly covering the changes as far as I can tell. In fact, this PR is aligning the tensor backend with the PIL backend, which was already expanding 1D images to 3D. I'm gonna mark this as a bugfix.
Thank you!
| if num_output_channels not in (1, 3): | ||
| raise ValueError(f"num_output_channels must be 1 or 3, got {num_output_channels}.") |
There was a problem hiding this comment.
This check is already done in all of the call sites of this helper, so it's not needed here. In general I agree we should error at the lower-level functions like this one instead of earlier. Maybe it's be better to error here instead of the callsites, but since it would require more change than necessary for this feature, let's leave it out for now.
There was a problem hiding this comment.
Replaced with a TODO. Let me know if something else is needed here.
| if image.shape[-3] == 1 and num_output_channels == 3: | ||
| s = [-1] * len(image.shape) | ||
| s[-3] = 3 | ||
| image = image.expand(s) |
There was a problem hiding this comment.
Should we simply do
| image = image.expand(s) | |
| return image.expand(s) |
here? The output should be the same and it would save some extra computation from l_img = r.mul(0.2989).add_(g, alpha=0.587).add_(b, alpha=0.114). Not critical though.
There was a problem hiding this comment.
You are right. This saves some ops. Done.
|
Turns out I am not sure that's the behavior that the user wants/expects. I changed to |
|
Good catch @ahmadsharif1 , thank you. |
Summary: Co-authored-by: Nicolas Hug <[email protected]> Reviewed By: vmoens Differential Revision: D55062762 fbshipit-source-id: 8a36b5ada2a5926b5280b0dc420efb41ecbb3fa1
Before this patch, if we passed in an image with just one channel and requested
num_output_channels=3, we would return an image with a single channel. After this patch, we will expand the channels to 3 if requested.This issue is described in #8167 .
@NicolasHug could you give some guidance on how to test this more thoroughly?
cc @vfdev-5