-
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
[BC-breaking] Aligning fill = None
with fill = 0
on Transforms
#6623
Comments
Trying to understand this more, is the problem that:
If we were to have |
-> answer is both 3rd alternative is: to try to put fill=0 in functional ops and make fill=0 behave as fill=None. Checking the codebase history, I have an impression is that default fill 0 or None has its root in PIL API: Fill argument exists for pad, affine, rotate and others affine-like functional ops.
So, making output result for all the ops the same if fill=None and fill=0, would be a move forward. |
@vfdev-5 Thanks for the comprehensive reply. Do you think I should add the alternative as a 3rd option? Or perhaps I could just edit the "proposal" to suggest that we should be using |
Thanks for the details @vfdev-5 .
I'm looking at #6517 but I can't pin-point exactly what the difference is. Would you have more details? Is it that the "filling" simply doesn't happen when Also instead of a BC breaking change, is deprecation an option here? |
Yeah, this issue description may be unclear. The problem arises when we call Why results are not matching for bilinear ? Because when fill is None we do only grid sampling (which does padding_mode="zeros"). If fill=0 we create additional mask channel, concat it to the input image, then grid sample, extract transformed mask and apply it with fill values (which are 0 again) to the output image. The way how we blend mask with fill value and the image is probably incorrect for bilinear mode (see images in the issue, there is a black shadow around rotated image for current torchvision. This shadow is absent for PIL). So, the problem comes from blending zero fill values to already transformed image with zero fill. Before fixing blending, we may ask why we bother doing all this mask fancy stuff for fill=0 if grid sample already does that for us. So, I would expect that fill=0 should be a no-op like fill=None in terms of computing mask and manually applying fill value. Thus, point 1 in the issue is about making fill=0 option as like fill=None without computing any fill mask. |
Just had a chat with @vfdev-5 and @pmeier , I'll try to briefly summarize the situation on V1 transforms for my own understanding (possibly re-hashing what has already been said above). These are general statements that do not necessarily apply to all transforms in all cases (sometimes, only a subset of them):
All of these are issues we'll need to fix eventually. We don't know how yet. As discussed in #7258, the V2 transform introduce a minor BC-breaking change by not doing this anymore:
I.e. if users pass None to a transform, the functional will now get None as well in V2. This allows users who explicitly pass |
To narrow it down a little, this only happens for
#6517 goes into details, but here is the TL;DR: PyTorch's |
🚀 The feature
As discussed at #6517, passing
None
onfill
for some transforms isn't the same as passing0
. The difference applies on some cases but not others, it was potentially done to avoid JIT issues (or is it a bug?) and has unforeseen effects on the behaviour of theTransforms
.Currently on the prototype area:
fill=None
as default value.fill=0
as default.fill=None
.The above inconsistent behaviour maintains BC with stable but leads to a weird API.
Motivation, pitch
Break BC on the
functional
level and makefill=None
behave similar tofill=0
.This unfortunately will have accuracy effects on models trained with the Tensor API. Worth noting that such a change would align closer with PIL which already sets
fill=None
to 0:vision/torchvision/transforms/functional_pil.py
Lines 264 to 265 in 56e707b
This proposal could be adopted if we decide to break BC and align the
Tensor
andPIL
backends on other aspects (such asantialias
etc).Alternatives
There are 2 other potential alternatives:
Additional context
No response
cc @vfdev-5
The text was updated successfully, but these errors were encountered: