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.

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

Added missing typing annotations in datasets/_stereo_matching #6846

Merged
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

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

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

and all other occurrences of this more precise?

@frgfm frgfm requested a review from pmeier November 1, 2022 16:04
@@ -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 😅

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

torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
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.

torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
torchvision/datasets/_stereo_matching.py Outdated Show resolved Hide resolved
@frgfm frgfm requested a review from pmeier December 13, 2022 20:16
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!

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.

@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
#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
Labels
cla signed other if you have no clue or if you will manually handle the PR in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants