it seems reasonable to do this

Do we gain much by storing the RNG? Could that be avoided?

I feel like you can't avoid storing it, because it is used in Base.iterate. Passing it in as state would be too clunky.

I think I'm wondering whether we are better off supporting custom iterators Instead and have these kinds of details handled by the that function instead

i don't know what you mean, DataLoader is a custom iterator already

Let me do a rough draft of what I have in mind tonight

This seems like a small, simple change that would be nice to get in before the next release. It's breaking right? Would be a shame to wait until 0.13 to use user-specified RNGs.

It's breaking right?

doesn't look breaking, just another keyword arg with the default argument which was implicitly used already

After thinking about this a bit more, why not direct users to something like MLDataPattern.shuffleobs that already takes an RNG argument? DataLoader has always been awkward when it comes to extensibility (i.e. it isn't at all), but I'm not sure the solution is to add more arguments (I'll refrain from rambling about DataLoader vs DataLoaders.DataLoader here 😛).

I think we should hold-off on pushing MLDataPattern.jl too much until it has been refactored (i.e. MLDataPattern has a DataLoader that can replaces DataLoaders.DataLoader). This is fairly simple and easy feature add.

PS: wow that sentence was a mouthful

I would want the whole DataLoader to be extensible, so we can use it for things like training GANs/ NODEs, tuples of arguments (unlike just working on arrays), sending data to different workers/ devices asynchronously etc. It would be cool to have a package to take care of the heavy lifting here, but failing that, I would want these features before moving ahead with MLData* packages.

This should be its own issue by the looks of it.

bors r+

@CarloLucibello there's pending comments about lacking tests and this is an api change. Further, regardless of merging this or #1508, the comments need to be addressed. We were not in a rush for merging this, anyway.

Build succeeded:

• buildkite/flux-dot-jl