add grid transform zero fill shortcut #8099
Conversation
| # filling with zeros is the default behavior and thus we can skip the extra fill handling | ||
| if fill is not None and all(f == 0 for f in fill): | ||
| fill = None |
There was a problem hiding this comment.
Maybe it's because I should be sleeping already but I don't really understand the meaning of that comment, or what that code is really doing...
Is fill = None supposed to be equivalent to filling with zeros?
There was a problem hiding this comment.
Is
fill = Nonesupposed to be equivalent to filling with zeros?
Yes. IMO the parameter name "fill" is a misnomer. Filling will happen for most affine ops whether the user likes it or not and regardless of what value they pass for fill. This is just the nature of affine transformations since they can leave the original image into a state that doesn't fill the full rectangular image. Hence we need to fill the parts that are not covered by the transformed image.
IMO, a better name would be "fill color". We had that initially, but it was decided for fill in #2479. IIUC, the only reason for it was that we had more ops that used fill than ops that used fillcolor 🤷
Coming back to your question: yes fill=None, fill=0, and fill=[0,0,0] in theory should all do the same thing, namely filling with zeros. I don't know why we chose None as default rather than fill=0. IMO that further enhances the confusion that there is actually a case that doesn't fill.
Anyway, fill=None is a performance optimization. The PyTorch op that we use does not allow filling with anything but zeros (other filling modes are supported, but we need to change the fill value for constant filling). If we get a any value besides fill=None, we perform some extra steps to allow the fill color the user wants. However in case of fill=0 or fill=[0,0,0] this unnecessary, since it is the default behavior from the PyTorch op anyway.
Right now there is a bug (?) in our affine ops that actually gives us different behavior for the case of fill is not None. #8098 deals with that. If that is resolved, this PR will just be a performance shortcut. If we decide to not move forward with #8098, this likely means, we should also close this PR.
Fixes #6517 together with #8098.
cc @vfdev-5