Add 5 arg copyto! into Vector with matching type
#264
Conversation
src/array.jl
Outdated
| @@ -76,6 +89,9 @@ macro implement_array_methods(t) | |||
| function Base.copyto!(dest::PermutedDimsArray{T,N}, src::$t{T,N}) where {T,N} | |||
| return $_disk_copyto!(dest, src) | |||
| end | |||
| function Base.copyto!(dest::Vector{T}, dstart::Integer, src::$t{T, 1}, sstart::Integer, n::Integer) where {T} | |||
There was a problem hiding this comment.
Is T always identical for scr and dest in Base copyto! ? What if you have different Real types
There was a problem hiding this comment.
No, the copyto! in Base uses setindex! so will call convert on each element.
I added a version that uses broadcasting to support that case.
There was a problem hiding this comment.
I'm assuming here that readblock! requires the element types to match, and aout to be stored in contiguous memory.
There was a problem hiding this comment.
We also use setindex! when writing into the chunk buffer, so it should be the same?
There was a problem hiding this comment.
Maybe. Looking at the docs, it just says "The results shall be written into aout" but I'm not sure if there are implementations of readblock! that make other assumptions about aout's eltype, or memory layout.
There was a problem hiding this comment.
Julia does automatic type conversion when writing to an array. (And dest is a Vector)
If any other package breaks that we will fix it on that side.
src/array.jl
Outdated
| throw(ArgumentError(LazyString("tried to copy n=", | ||
| n," elements, but n should be non-negative"))) | ||
| end | ||
| view(dest, range(dstart, length=n)) .= view(src, range(dstart, length=n)) |
There was a problem hiding this comment.
Depending on the use case it might be better for performance to avoid the DiskArray broadcast machinery, broadcast on views should work but might be a bit slow if chunk sizes are too small. Instead we might instantiate the result and copy with coptyo!(dest,dstart,src[range(dstart, length=n)])
Can we assume that the amount of data we copy fits in memory, or is dest mmapped in your case?
There was a problem hiding this comment.
I'm not doing any mmapping in InputBuffers.jl, though there is nothing stopping someone from passing a mmapped out to Base.readbytes!(b::InputBuffer, out::AbstractArray{UInt8}, nb=length(out))
Also, the copyto! in InputBuffers.jl should end up calling _disk_copyto_same_type_vector with the direct readblock! call since src and dest both have eltype UInt8.
There was a problem hiding this comment.
I would also prefer @meggart s approach, although you can use a view rather than get index on src. But let's try to avoid nested disk broadcasts
nhz2
changed the title
copyto! into Vectorcopyto! into Vector with matching type
|
I removed the specialization that does type conversion to limit the scope of this PR. |
|
Bump |
|
I would like this to work on e.g. Float64 dest from a Float32 diskarray. Dest is an |
Fixes #263
I only implemented the one-dimensional case because it wasn't obvious to me how to handle multiple dimensions, and this is all that is required for better
InputBuffers.jlcompatibility for now.I also modified the
copyto!function to always returndest, which somehow fixed thecircshifttest.