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

Fix inline typing for mypy #544

Merged
merged 8 commits into from Apr 21, 2020

Conversation

tomassosorio
Copy link
Contributor

@tomassosorio tomassosorio commented Apr 14, 2020

relates to #477
This should fix all mypy issues and could be used to check if PR follow all its agreements running:
mypy torchaudio/
getting:
Success: no issues found in 19 source files

@tomassosorio tomassosorio changed the title Fix inline typing for mypy [WIP] Fix inline typing for mypy Apr 14, 2020
@tomassosorio tomassosorio changed the title [WIP] Fix inline typing for mypy Fix inline typing for mypy Apr 14, 2020
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

Thanks for correcting some of the typing.

I suggest we do not yet annotate new variable definition for now. I'd be ok with skipping this error in mypy.

@@ -168,13 +168,13 @@ def save_encinfo(filepath: str,
# sox stores the sample rate as a float, though practically sample rates are almost always integers
# convert integers to floats
if signalinfo:
if not isinstance(signalinfo.rate, float):
if signalinfo.rate and not isinstance(signalinfo.rate, float):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is trying to avoid converting signalinfo.rate when it's zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually this is due to SignalInfo is defined as None in its variables.
So this will check if the variable exists if so then will check if the variable isinstance float

if float(signalinfo.rate) == signalinfo.rate:
signalinfo.rate = float(signalinfo.rate)
else:
raise TypeError('Sample rate should be a float or int')
# check if the bit precision (i.e. bits per sample) is an integer
if not isinstance(signalinfo.precision, int):
if signalinfo.precision and not isinstance(signalinfo.precision, int):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

from torch.utils.data import Dataset
from torch.utils.model_zoo import tqdm


def unicode_csv_reader(unicode_csv_data: TextIOWrapper, **kwargs: Any) -> str:
def unicode_csv_reader(unicode_csv_data: TextIOWrapper, **kwargs: Any) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not BC breaking yet since the inline typing isn't part of the release. str was added in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was changed due to csv.reader has as output Any instead of str so this function actually will give Any and not solely str has output

torchaudio/datasets/utils.py Outdated Show resolved Hide resolved
torchaudio/datasets/utils.py Outdated Show resolved Hide resolved
torchaudio/datasets/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM

@vincentqb vincentqb merged commit 9d40302 into pytorch:master Apr 21, 2020
@tomassosorio tomassosorio deleted the fixInlineTypingForMypy branch April 21, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants