Skip to content
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

Given that DataLoader implements length shouldn't it also be able to provide size? #2372

Closed
FelixBenning opened this issue Jan 25, 2024 · 4 comments · Fixed by JuliaML/MLUtils.jl#174

Comments

@FelixBenning
Copy link

function Base.length(d::DataLoader)
n = d.nobs / d.batchsize
d.partial ? ceil(Int,n) : floor(Int,n)
end

ERROR: MethodError: no method matching size(::MLUtils.DataLoader{Tuple{Array{Float32, 3}, OneHotArrays.OneHotMatrix{UInt32, Vector{UInt32}}}, Random._GLOBAL_RNG, Val{nothing}})
Closest candidates are:
  size(::Union{LinearAlgebra.QR, LinearAlgebra.QRCompactWY, LinearAlgebra.QRPivoted}) at /Applications/Julia-1.8.app/Contents/Resources/julia/share/julia/stdlib/v1.8/LinearAlgebra/src/qr.jl:581
  size(::Union{LinearAlgebra.QR, LinearAlgebra.QRCompactWY, LinearAlgebra.QRPivoted}, ::Integer) at /Applications/Julia-1.8.app/Contents/Resources/julia/share/julia/stdlib/v1.8/LinearAlgebra/src/qr.jl:580
  size(::Union{LinearAlgebra.Cholesky, LinearAlgebra.CholeskyPivoted}) at /Applications/Julia-1.8.app/Contents/Resources/julia/share/julia/stdlib/v1.8/LinearAlgebra/src/cholesky.jl:514

The size is requested by the @progress macro of ProgressLogging.jl

@re1san
Copy link

re1san commented Jan 28, 2024

Hey, I would like to work on it. If I am understanding it correctly there should be a function Base.size(d::DataLoader) that returns the same thing as length right?

@ToucheSir
Copy link
Member

size is defined to return a tuple, instead of a single int, so almost. I'd have a look at the docs/REPL help for size and try it out on a couple of different AbstractVector subtypes to get a feel for how it should be implemented.

@mcabbott
Copy link
Member

Note BTW that the code for this lives in MLUtils.jl, not in the Flux.jl repository: https://github.com/JuliaML/MLUtils.jl/blob/main/src/eachobs.jl#L186

Should be an easy PR to add this, Base.size(d::DataLoader) = (length(d),) and a test.

At least if I'm thinking correctly. The size should always be the size of collect(d), and this will always be a 1-dimensional container, Vector, I think.

@re1san
Copy link

re1san commented Feb 7, 2024

Where should the tests be added? Should it be added in https://github.com/JuliaML/MLUtils.jl/blob/main/test/dataloader.jl?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants