Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added missing typing annotations in datasets/_stereo_matching #6846

Merged
merged 12 commits into from
Dec 21, 2022
Merged

Added missing typing annotations in datasets/_stereo_matching #6846

merged 12 commits into from
Dec 21, 2022

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Oct 27, 2022

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

Any feedback is welcome!

cc @datumbox @pmeier @oke-aditya

@frgfm frgfm mentioned this pull request Oct 27, 2022
Closed
@pmeier pmeier self-requested a review October 27, 2022 11:15
pmeier
pmeier reviewed Oct 27, 2022
View reviewed changes
Copy link
Collaborator

@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 Francois. I don't have any inline comments, but could you also make

vision/torchvision/datasets/_stereo_matching.py

Line 88 in 52b80c4

def _read_disparity(self, file_path: str) -> Tuple:

and all other occurrences of this more precise?

frgfm added 3 commits November 1, 2022 15:38
@frgfm
Merge branch 'main' into typing-stereomatching

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
c77c282
@frgfm
Merge branch 'typing-stereomatching' of github.com:frgfm/vision into …
9e08262 
…typing-stereomatching
@frgfm
style: Specified typing
f5f8d40
@frgfm frgfm requested a review from pmeier November 1, 2022 16:04
oke-aditya
oke-aditya reviewed Nov 1, 2022
View reviewed changes
torchvision/datasets/_stereo_matching.py
@@ -14,6 +14,9 @@
from .utils import _read_pfm, download_and_extract_archive, verify_str_arg
from .vision import VisionDataset

T1 = Tuple[Image.Image, Image.Image, Optional[np.ndarray], np.ndarray]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice way to keep things simple

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. I wonder if we can make the name more expressive though. Not blocking, but T1 and T2 are rather opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but I tried to think about a meaning naming and honestly I didn't manage to find any 😅

pmeier
pmeier reviewed Nov 1, 2022
View reviewed changes
torchvision/datasets/_stereo_matching.py
@@ -14,6 +14,9 @@
from .utils import _read_pfm, download_and_extract_archive, verify_str_arg
from .vision import VisionDataset

T1 = Tuple[Image.Image, Image.Image, Optional[np.ndarray], np.ndarray]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. I wonder if we can make the name more expressive though. Not blocking, but T1 and T2 are rather opaque.

pmeier
pmeier reviewed Nov 8, 2022
View reviewed changes
Copy link
Collaborator

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

Sorry for the late reply @frgfm.

@frgfm frgfm requested a review from pmeier December 13, 2022 20:16
pmeier
pmeier approved these changes Dec 21, 2022
View reviewed changes
Copy link
Collaborator

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

LGTM, thanks @frgfm!

pmeier
pmeier requested changes Dec 21, 2022
View reviewed changes
Copy link
Collaborator

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

Lint job is failing: https://app.circleci.com/pipelines/github/pytorch/vision/22404/workflows/f6cbd044-6379-480a-bd93-2352646e4c2b/jobs/1800508. Afterwards, this should be good to merge.

@frgfm frgfm requested a review from pmeier December 21, 2022 11:43
@pmeier pmeier merged commit b0aa60c into pytorch:main Dec 21, 2022
@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

@pmeier pmeier added the other if you have no clue or if you will manually handle the PR in the release notes label Dec 21, 2022
@frgfm frgfm deleted the typing-stereomatching branch December 21, 2022 13:50
facebook-github-bot pushed a commit that referenced this pull request Jan 9, 2023
@pmeier @facebook-github-bot
[fbsync] Added missing typing annotations in datasets/_stereo_matching (
775244c 
#6846)

Summary:
* style: Added missing typing annotations in datasets/_stereo_matching

* style: Specified typing

* style: Specified type annotations further

* style: Specified typing of __getitem__

* style: Fixed linting

Reviewed By: NicolasHug

Differential Revision: D42414489

fbshipit-source-id: a9127b783fcccb0c17b28487ce63fe1a50d61dcb

Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oke-aditya oke-aditya

@pmeier pmeier

Assignees
No one assigned
Labels
cla signed other if you have no clue or if you will manually handle the PR in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@frgfm @pmeier @oke-aditya @facebook-github-bot