Another way to deal with splitting #60
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
I like these changes, will update my dataloader to fit with these changes and separate out the downloading. Then we only downlaod the MNIST once @hzavadil98. |
sot176
reviewed
Feb 10, 2025
Contributor
sot176
left a comment
sot176
left a comment
There was a problem hiding this comment.
I think that is a great way to separate out the downloading from the dataset class. It makes the code more cleaner :)
I will adjust my dataset class to this new format!
Contributor
|
Nice work 👍 I added mnist downloader and adjusted my dataset. I also had to make a few minor changes with parameter passing which got a bit more complicated. I will now merge with my previous PR so that there is only one spot to push the changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned in my response to #53, here are a few changes I think we could consider that might simplify the splitting a bit.
Thoughts?
Note that this pull-request is not to merge with main, but with @hzavadil98's dataloader pull request