-
Notifications
You must be signed in to change notification settings - Fork 25.7k
fix RandomSampler length #15991
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 RandomSampler length #15991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct patch as discussed in the issue: the dataset now returns the correct length in case num_samples is specified. Also good that there are now some tests in place which prevent a regression issue to popup in the future. Thanks!
torch/utils/data/sampler.py
Outdated
|
|
||
| @property | ||
| def num_samples(self): | ||
| # work round around growing dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be clearer to say that “dataset size might change at runtime”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apaszke thanks for a great advice. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me thanks!
Could you just address @apaszke comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Hi! This PR addresses #15537 issue. Please review. Thanks! Differential Revision: D13649890 Pulled By: soumith fbshipit-source-id: 166212ae383331345423236dfc4fa2ea907d265d
|
Merged in a741578 |
Hi!
This PR addresses #15537 issue.
Please review.
Thanks!
cc: @fmassa @jbaron @soumith