Skip to content

Conversation

@krishnakalyan3
Copy link
Contributor

@krishnakalyan3 krishnakalyan3 commented Dec 12, 2020

I have also addressed your comments on torch.random #1022 (comment).
Fixes #1081

@mthrok
Copy link
Contributor

mthrok commented Dec 14, 2020

Thanks for working on this but you need to get rid of all the internal use.
Please run tests by pytest -vs test/torchaudio_unittest/datasets/

@krishnakalyan3
Copy link
Contributor Author

Hi, @mthrok looks like I am missing some context. Can you please let me know why unicode_csv_reader should be removed?.
If removed what should I be replacing it with?.

@mthrok
Copy link
Contributor

mthrok commented Dec 17, 2020

These utilities were added (for internal use) to ease the difference between Python 2 and Python 3.

But now that Python 2 support is officially dropped, we do not need these utilities. Instead we can use the standard module as-is.
In Python 3 os.makedirs function has exist_ok argument, and csv module can handle unicode natively without the hack of changing the field_limit_size.

So we just need to replace the usages of these functions with standard module. Then we can remove the utility functions themselves.

@krishnakalyan3
Copy link
Contributor Author

@mthrok can you please review this.

Copy link
Contributor

@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. Thank you so much!

@mthrok mthrok merged commit 3cd5eed into pytorch:master Dec 19, 2020
@krishnakalyan3 krishnakalyan3 deleted the remove_utils branch December 19, 2020 21:10
@krishnakalyan3
Copy link
Contributor Author

@mthrok can you please recommend some other (easy - medium complexity) issues that I could work on?.

Cheers,
Krishna

@mthrok
Copy link
Contributor

mthrok commented Dec 21, 2020

@krishnakalyan3 Sure. Thanks for offering :) Let me think of an issue. Meanwhile, if you can join PyTorch's slack channel, we can discuss collaboration there too. Communication will be faster that way. https://pytorch.org/resources/

@mthrok
Copy link
Contributor

mthrok commented Dec 22, 2020

@krishnakalyan3

How about #910 (comment) ?
I consider this as not easy in terms of complexity because there are somewhat open-ended perspectives in the specification, so we need to iterate on the design.

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
* fix device mismatch issue pytorch#1071

* fix device mismatch issue pytorch#1071

* add mnist_rnn to test script for CI

* support dry_run in test()
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.

Remove redundant dataset utilities

3 participants