-
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
fix bug when using PIL backend in references/classification #7665
fix bug when using PIL backend in references/classification #7665
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7665
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit f776906: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @AetelFinch! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
@@ -69,11 +69,10 @@ def __init__( | |||
backend="pil", | |||
): | |||
trans = [] | |||
|
|||
backend = backend.lower() | |||
if backend == "tensor": |
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.
Probably this should change to
if backend == "tensor": | |
if backend == "tensor" or backend == "pil": |
backend = backend.lower() | ||
if backend == "tensor": | ||
trans.append(transforms.PILToTensor()) | ||
else: | ||
elif backend != "pil": |
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.
This probably still be buggy when backend="pil"
As it will not apply PILtoTensor()
transform.
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.
Should we apply PILtoTensor when backend="pil"? I was just guided by the way it is done in the ClassificationPresetTrain class
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.
We probably do this in Line 83. I missed this. But it's confusing why would one apply PILToTensor()
when backend type is Tensor()
. Maybe I'm missing something again.
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.
As I see, we apply PILtoTensor at the beginning, when backend=="tensor", to do transformations over tensors, not over PIL images.
trans += [
transforms.Resize(resize_size, interpolation=interpolation, antialias=True),
transforms.CenterCrop(crop_size),
]
As written in the documentation for transforms.Resize:
"The output image might be different depending on its type: when downsampling, the interpolation of PIL images and tensors is slightly different, because PIL applies antialiasing. This may lead to significant differences in the performance of a network."
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's confusing why would one apply PILToTensor() when backend type is Tensor().
Yeah it's not super clean nor obvious just reading the code. The key piece of information is that those presets make the hard assumption that whatever you pass as input is a PIL image, no matter the backend!
So when we pass backend="tensor"
we actually need to first convert the input (a PIL image) to a tensor.
A more complete solution would be to do all those checks "at runtime" in forward()
instead of here.
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.
Thanks @AetelFinch , LGTM, will merge when green
Hey @NicolasHug! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
…7665) Summary: Co-authored-by: Max Chuprov <m.chuprov@expasoft.tech> Reviewed By: vmoens Differential Revision: D46724127 fbshipit-source-id: 300232aadc8fe22ece46c229ec59ac0b95784756
fix #7664