Skip to content

Added typing annotations to models/segmentation #4227

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

Merged
merged 8 commits into from
Aug 23, 2021

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jul 29, 2021

Following up on #2025, this PR adds missing typing annotations in models/segmentation.

Note to reviewers: I'm having trouble with the return types of the segmentation factory functions 🤷‍♂️

Any feedback is welcome!

cc @datumbox

@frgfm frgfm changed the title Added typing annotations to models/segmentation/segmentation Added typing annotations to models/segmentation Jul 29, 2021
@frgfm
Copy link
Contributor Author

frgfm commented Jul 29, 2021

Docstring issue handled in #4228

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@frgfm Apologies for the delayed review. I had a look and added a few comments. Let me know what you think.

@frgfm frgfm requested a review from datumbox August 23, 2021 08:13
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Thanks @frgfm! I think we are almost there.

Could you have a quick look so that we can get this merged today?

@frgfm frgfm requested a review from datumbox August 23, 2021 09:43
aux: bool,
aux: Optional[bool],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not strict, but isn't Optional constrained to argument with specified default value being None?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because it can get None value from the calling methods... Mypy was complaining about it.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM @frgfm, thanks a lot for your work and patience.

Will merge once tests pass.

@datumbox datumbox force-pushed the segmentation-typing branch from 86d5a23 to 9eaddd9 Compare August 23, 2021 11:30
@datumbox datumbox merged commit 185be3a into pytorch:main Aug 23, 2021
@frgfm frgfm deleted the segmentation-typing branch August 23, 2021 12:15
facebook-github-bot pushed a commit that referenced this pull request Aug 26, 2021
Summary:
* style: Added typing annotations to segmentation/_utils

* style: Added typing annotations to segmentation/segmentation

* style: Added typing annotations to remaining segmentation models

* style: Fixed typing of DeepLab

* style: Fixed typing

* fix: Fixed typing annotations & default values

* Fixing python_type_check

Reviewed By: fmassa

Differential Revision: D30525894

fbshipit-source-id: 9926506955fba566ce61d17274ff7d86ed496383
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.

4 participants