-
Notifications
You must be signed in to change notification settings - Fork 7.1k
make fill defaultdict an implementation detail #7258
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
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.
Minor comments but LGTM, thanks Philip
In the transforms v1 tests we have vision/test/test_transforms.py Lines 1871 to 1872 in d805aea
That will still not be supported by this change. I don't really understand the whole |
This story is certainly a tradeoff between what was supported at that moment supported values (by Pillow, pytorch) and jit, consistency etc. For RandomRotation fill arg, it seems like the story was like that:
Today there are still few minor issues and inconcistencies for fill arg:
Also check this comment: #6623 (comment) |
Reviewed By: vmoens Differential Revision: D44416563 fbshipit-source-id: 3ac6e3f6a7b6cfa4766c0a4a50b643d47a35e265
Towards #7159. For v2 we added more complex handling for
fill
to enable filling with different values based on the type. Internally this is handled by adefaultdict
:vision/torchvision/prototype/transforms/_utils.py
Line 68 in d805aea
However, there is no need to leak that into a public attribute:
vision/torchvision/prototype/transforms/_geometry.py
Line 280 in d805aea
This PR simply leaves parameter the user passed in the
self.fill
attribute and moves thedefaultdict
toself._fill
cc @vfdev-5 @bjuncek