KEMBAR78
fix RandomSampler length by truskovskiyk · Pull Request #15991 · pytorch/pytorch · GitHub
Skip to content

Conversation

@truskovskiyk
Copy link
Contributor

@truskovskiyk truskovskiyk commented Jan 12, 2019

Hi!

This PR addresses #15537 issue.
Please review.

Thanks!

cc: @fmassa @jbaron @soumith

Copy link

@jbaron jbaron left a 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!


@property
def num_samples(self):
# work round around growing dataset
Copy link
Contributor

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”

Copy link
Contributor Author

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.

Copy link
Member

@fmassa fmassa 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 to me thanks!
Could you just address @apaszke comments?

@truskovskiyk
Copy link
Contributor Author

@fmassa Thanks for approving. I already have fixed @apaszke comments.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

facebook-github-bot pushed a commit that referenced this pull request Jan 14, 2019
Summary:
Hi!

This PR addresses #15537  issue.
Please review.

Thanks!

Differential Revision: D13649890

Pulled By: soumith

fbshipit-source-id: 166212ae383331345423236dfc4fa2ea907d265d
@fmassa
Copy link
Member

fmassa commented Jan 14, 2019

Merged in a741578

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.

6 participants