Skip to content

Conversation

@RoiEXLab
Copy link
Contributor

@RoiEXLab RoiEXLab commented Jan 15, 2023

Hi,
just a small change to make automatic type inheritance a little bit more pleasant for some people.

cc @vfdev-5 @datumbox

@facebook-github-bot
Copy link
Contributor

Hi @RoiEXLab!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RoiEXLab. One comment inline that needs to be addressed. Otherwise LGTM if CI is happy.

def __init__(self, brightness=0, contrast=0, saturation=0, hue=0):
def __init__(
self,
brightness: "float | tuple[float, float]" = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, using the | operator for Union is only supported starting Python 3.10. See PEP604 for details. Is this the reason you are using string annotations here?

Copy link
Contributor Author

@RoiEXLab RoiEXLab Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. I think using tuple[float] instead of Tuple[float] from typings does not work on python 3.8 either (which I'm still using).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, our minimum version is 3.7 so we'll have to abide by that.

@RoiEXLab
Copy link
Contributor Author

@pmeier Changed the hints accordingly. Should work now. Some of the CI on some systems is failing, but I'm pretty sure it's not related to this PR

@pmeier
Copy link
Contributor

pmeier commented Jan 17, 2023

The CircleCI failures are unrelated to this PR. I've started the GHA ones. Let's see if everything is green there as well. If so, we can merge.

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failures are unrelated. Thanks @RoiEXLab!

@pmeier pmeier merged commit 93df9a5 into pytorch:main Jan 17, 2023
@github-actions
Copy link

Hey @pmeier!

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

@RoiEXLab RoiEXLab deleted the patch-1 branch January 17, 2023 13:13
facebook-github-bot pushed a commit that referenced this pull request Jan 24, 2023
Reviewed By: YosuaMichael

Differential Revision: D42706910

fbshipit-source-id: 93184ccbd1e81a20d04834244457870c32dde768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants