Skip to content

Add missing None type hint to init functions #6354

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ProGamerGov
Copy link
Contributor

@ProGamerGov ProGamerGov commented Aug 2, 2022

All __init__ functions should have the type hint -> None according to PEP-0484: https://www.python.org/dev/peps/pep-0484/

Copy link
Contributor

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

As such many types are missing 😓 In other files. I just added comment as we can atleast fix up the init in this PR.

Maybe I should get back and try fixing the typing PRs?

cc @datumbox @NicolasHug ?

@@ -118,7 +118,7 @@ class Sintel(FlowDataset):
return a built-in valid mask, such as :class:`~torchvision.datasets.KittiFlow`.
"""

def __init__(self, root, split="train", pass_name="clean", transforms=None):
def __init__(self, root, split="train", pass_name="clean", transforms=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Well let's type this correctly?

Suggested change
def __init__(self, root, split="train", pass_name="clean", transforms=None) -> None:
def __init__(self, root: str, split: str = "train", pass_name: str ="clean", transforms: Optional[Callable] = None) -> None:

@@ -180,7 +180,7 @@ class KittiFlow(FlowDataset):

_has_builtin_flow_mask = True

def __init__(self, root, split="train", transforms=None):
def __init__(self, root, split="train", transforms=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same flows for all comments

Suggested change
def __init__(self, root, split="train", transforms=None) -> None:
def __init__(self, root: str, split: str = "train", transforms: Optional[Callable] = None) -> None:

@@ -245,7 +245,7 @@ class FlyingChairs(FlowDataset):
return a built-in valid mask, such as :class:`~torchvision.datasets.KittiFlow`.
"""

def __init__(self, root, split="train", transforms=None):
def __init__(self, root, split="train", transforms=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, root, split="train", transforms=None) -> None:
def __init__(self, root: str, split: str = "train", transforms: Optional[Callable] = None) -> None:

@@ -316,7 +316,7 @@ class FlyingThings3D(FlowDataset):
return a built-in valid mask, such as :class:`~torchvision.datasets.KittiFlow`.
"""

def __init__(self, root, split="train", pass_name="clean", camera="left", transforms=None):
def __init__(self, root, split="train", pass_name="clean", camera="left", transforms=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, root, split="train", pass_name="clean", camera="left", transforms=None) -> None:
def __init__(self, root: str, split: str = "train", pass_name:str = "clean", camera: str = "left", transforms: Optional[Callable] = None) -> None:

@@ -401,7 +401,7 @@ class HD1K(FlowDataset):

_has_builtin_flow_mask = True

def __init__(self, root, split="train", transforms=None):
def __init__(self, root, split="train", transforms=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, root, split="train", transforms=None) -> None:
def __init__(self, root: str, split: str = "train", transforms: Optional[Callable] = None) -> None:

@@ -111,7 +111,7 @@ class NormalizeVideo:
inplace (boolean): whether do in-place normalization
"""

def __init__(self, mean, std, inplace=False):
def __init__(self, mean, std, inplace=False) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyways we plan to remove this file in next version of torchvision.

@oke-aditya
Copy link
Contributor

Should I get back and work on #4630 #4612 #4599 ?

@NicolasHug
Copy link
Member

Thanks for the PR @ProGamerGov . I'm not too keen on merging this one, because simply adding -> None to most methods / functions doesn't really add a lot of value to the code base, it's just extra characters.

As such many types are missing sweat In other files. I just added comment as we can atleast fix up the init in this PR.

Maybe I should get back and try fixing the typing PRs?

cc @datumbox @NicolasHug ?

These typing PRs add very little value to the code-base, and even sometimes negative values. Considering how much time consuming it is to write, review, and then maintain them, I'd recommend spending time on more impactful areas.

@oke-aditya
Copy link
Contributor

Yeah I know typing is kind of always a debate. I somehow don't agree to having untyped code as investment in code quality and health is always worth while.

@NicolasHug
Copy link
Member

investment in code quality and health is always worth while.

Fully agreed. Good docs + good tests cover 90% of that already, for half of the effort it takes to work in an annotated and type-checked code-base.

@oke-aditya
Copy link
Contributor

Of course I agree to this.

Considering how much time consuming it is to write, review, and then maintain them, I'd recommend spending time on more impactful areas.

@datumbox
Copy link
Contributor

datumbox commented Aug 3, 2022

I had a look yesterday on the PR to confirm the only changes were the addition of -> None. Though I am usually in favour of typing annotations on methods to clarify the intended input, I too agree that this PR generates too many changes that don't add much value. PEP8 indeed suggests that constructors should be annotated with what you added but some of your changes caused mypy issues which you had to rollback at f5af5ab. So given our current setup, you weren't even able to make the suggested change in all places of the code without having the mypy complaining. For this reason, I am also hesitant to approve the PR but I'm not going to block if someone else wants to do it.

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.

5 participants