-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Fix docstring for 'fill' in transforms without 'padding_mode' #9171
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9171
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit e8571e3 with merge base ce5b26a ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
On that note though, please be careful with a few things about the change. Mainly, you've also altered the Also I think there's a misindentation on the |
I will keep that in mind and update the PR. |
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.
Agreed with @diaz-esparza that we need to make sure that when there is a padding_mode
parameter, we explicitly mention it in the docstring.
I will work on it. |
@@ -664,7 +664,7 @@ class RandomAffine(Transform): | |||
:class:`torchvision.transforms.InterpolationMode`. Default is ``InterpolationMode.NEAREST``. | |||
If input is Tensor, only ``InterpolationMode.NEAREST``, ``InterpolationMode.BILINEAR`` are supported. | |||
The corresponding Pillow integer constants, e.g. ``PIL.Image.BILINEAR`` are accepted as well. | |||
fill (number or tuple or dict, optional): Pixel fill value used when the ``padding_mode`` is constant. | |||
fill (number or tuple or dict, optional): Pixel fill value used for pixels outside the image boundary. |
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.
The indentation here is wrong, which is causing the docs builds to fail. I think it's just a missing space.
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.
Let's fix the spacing so that the docs build successfully.
I will do that. |
Summary
This PR applies the minimum docstring fix for
fill
, as discussed in #9149.Replaced:
With:
This corrects the docstring in transforms like
ElasticTransform
,RandomAffine
, etc., which do not have apadding_mode
parameter.No functional changes made.
Fixes: #9149
Hi @diaz-esparza — no worries at all, and thank you for your thoughtful message!
I’ve gone ahead and applied the minimum docstring fix as discussed in the issue, specifically removing the misleading reference to
padding_mode
and updating thefill
parameter description for clarity.You’re of course very welcome to iterate or build on top of it — especially if you’re still considering the more advanced version as a feature enhancement.
Appreciate your openness to feedback — and honestly, your explanation and tone were already clear and friendly! Looking forward to your thoughts on the PR