Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@parmeet
Copy link
Contributor

@parmeet parmeet commented Feb 4, 2022

Reference issue: #1493

@erip
Copy link
Contributor

erip commented Feb 5, 2022

LGTM!

@parmeet parmeet requested review from Nayef211 and vcm2114 February 5, 2022 17:46
Copy link
Contributor

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

Overall LGTM! I wonder if we should add the 2nd test for the split argument since this dataset also uses the @_wrap_split_argument decorator which means that it should be able to support both "train" and ("train",) for the split argument?

@parmeet
Copy link
Contributor Author

parmeet commented Feb 7, 2022

Overall LGTM! I wonder if we should add the 2nd test for the split argument since this dataset also uses the @_wrap_split_argument decorator which means that it should be able to support both "train" and ("train",) for the split argument?

I think the split argument is bit superficial since the data does not really have splits. We might want to just remove this argument from the dataset.

@parmeet parmeet merged commit 223584b into pytorch:main Feb 7, 2022
@parmeet parmeet deleted the test_cc100 branch February 7, 2022 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants