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

[Refactor][Dataset] YesNo implementation #1127

Merged
merged 9 commits into from
Jan 19, 2021

Conversation

krishnakalyan3
Copy link
Contributor

@krishnakalyan3 krishnakalyan3 commented Dec 24, 2020

Implementation for the YesNo dataset.

cc: @mthrok
Fixes: #910

The initial plan is to refactor the datasets (first part of the PR). Based on the complexity will create subsequent PRs.

@krishnakalyan3 krishnakalyan3 changed the title initial draft [Refactor] Dataset implementations Dec 24, 2020
torchaudio/datasets/yesno.py Outdated Show resolved Hide resolved
torchaudio/datasets/yesno.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. Please address my few comments and also please fix the style check issue.

torchaudio/datasets/yesno.py Outdated Show resolved Hide resolved
@mthrok
Copy link
Collaborator

mthrok commented Dec 27, 2020

@krishnakalyan3

The initial plan is to refactor the following datasets mentioned above in the first part of the PR.
Based on the complexity will create subsequent PRs.

Thanks, the code looks good. Since this PR is to aide the discussion, I will keep it as-is until we hear back on #910. (but because of the time of the year, I expect that we will not hear back very soon)

If you have extra time you would like to kill, then you can work on LJSPEECH too, but please keep it a separate PR. For CommonVoice currently #1126 is going on so you might want to hold off a bit.

@krishnakalyan3 krishnakalyan3 changed the title [Refactor] Dataset implementations [Refactor][Dataset] YesNo implementation Dec 28, 2020
@krishnakalyan3
Copy link
Contributor Author

@mthrok wishing you a merry Christmas and a happy new year :).
Thanks for the feedback. I have made the necessary corrections.

url: str = _RELEASE_CONFIGS["release1"]["url"],
folder_in_archive: str = _RELEASE_CONFIGS["release1"]["folder_in_archive"],
download: bool = False
) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mthrok I need your help again here. I am not sure how to resolve the style issue here. I am not exactly sure how to resolve this. This is what I was using.

Copy link
Collaborator

Choose a reason for hiding this comment

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

flake8 is no happy when indentation for continuous line is 4 spaces, because that is visually same as logical indentation. This often happens when if statement spans multiple lines or function indentation like this.

I think indenting 4 more spaces will resolve the issue.

def __init__(
        self,
        ...
) -> None:
    ...

@mthrok
Copy link
Collaborator

mthrok commented Dec 30, 2020

@mthrok wishing you a merry Christmas and a happy new year :).
Thanks for the feedback. I have made the necessary corrections.

@krishnakalyan3 thanks, wish you a happy holidays too :)

@mthrok mthrok merged commit f5aced8 into pytorch:master Jan 19, 2021
mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021
Co-authored-by: Nikita Shulga <[email protected]>
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.

Better structure dataset implementations
3 participants