Consolidate repr #5392
Consolidate repr #5392
Conversation
Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
💊 CI failures summary and remediationsAs of commit febfd09 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet.
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
torchvision/transforms/transforms.py
Outdated
| f"{f', translate={self.shear}' if self.shear is not None else ''}" | ||
| f"{f', translate={self.interpolation.value}' if self.interpolation != InterpolationMode.NEAREST else ''}" | ||
| f"{f', translate={self.fill}' if self.fill != 0 else ''}" | ||
| f"{f', translate={self.center}' if self.center is not None else ''}" |
There was a problem hiding this comment.
Nit: Ouch! I don't think this makes the code more readable.
torchvision/ops/deform_conv.py
Outdated
| f", stride={self.stride}" | ||
| f"{f', padding={self.padding}' if self.padding != (0, 0) else ''}" | ||
| f"{f', dilation={self.dilation}' if self.dilation != (1, 1) else ''}" | ||
| f"{f', groups={self.groups}' if self.groups != 1 else ''}" |
torchvision/ops/deform_conv.py
Outdated
| f"{f', padding={self.padding}' if self.padding != (0, 0) else ''}" | ||
| f"{f', dilation={self.dilation}' if self.dilation != (1, 1) else ''}" | ||
| f"{f', groups={self.groups}' if self.groups != 1 else ''}" | ||
| f"{', bias=False' if self.bias is None else ''}" |
There was a problem hiding this comment.
Did you miss a nested f?
| f"{', bias=False' if self.bias is None else ''}" | |
| f"{f', bias=False' if self.bias is None else ''}" |
There was a problem hiding this comment.
in this case there is no need because 'bias=False is just a string without placeholders
There was a problem hiding this comment.
Oh yes, you are right. Let's go with @NicolasHug's proposal below to avoid nesting and make the code easier to understand.
There was a problem hiding this comment.
missed this, but I already removed the nested fstrings (different from @NicolasHug proposal). Despite his proposal is more in line with the spirit of this PR I think we can merge as it is if you agree.
|
Thanks for the PR @jdsgomes , No strong opinion but we might be able to avoid the use of the use of double-f strings like f"{f', translate={self.interpolation.value}' if self.interpolation != InterpolationMode.NEAREST else ''}"by using s = "".join([
f"x={x} if x is not None" else "",
f"y={y} if y is not None" else "",
....
]) |
Thanks for the comment. I'm reverting this. |
|
Hey @jdsgomes! 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 |
Summary: * Consolidating __repr__ strings Reviewed By: NicolasHug Differential Revision: D34140258 fbshipit-source-id: c3fd233d3665d7310aacec9090e1e3d6a1dd2011 Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
Attempt to make the
__repr__uniform across the repo by:s.format(**self.__dict__)and making the variables we want to use explicit in the f-string