DiskArrays for Variable's
#205
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -309,85 +309,73 @@ end | |||||
| nomissing(a::AbstractArray,value) = a | ||||||
| export nomissing | ||||||
|
|
||||||
| function readblock!(v::Variable, aout, indexes::TI...) where TI <: Union{AbstractUnitRange,StepRange} | ||||||
|
There was a problem hiding this comment.
Suggested change
This dispatch was not correct so we were using the fallback for I get roughly the same performance as |
||||||
| datamode(v.ds) | ||||||
| _read_data_from_nc!(v, aout, indexes...) | ||||||
| return aout | ||||||
| end | ||||||
|
|
||||||
| function _read_data_from_nc!(v::Variable, aout, indexes::Int...) | ||||||
| aout .= nc_get_var1(eltype(v),v.ds.ncid,v.varid,[i-1 for i in reverse(indexes)]) | ||||||
| end | ||||||
|
|
||||||
| function _read_data_from_nc!(v::Variable{T,N}, aout, indexes::TR...) where {T,N} where TR <: Union{StepRange{Int,Int},UnitRange{Int}} | ||||||
| start,count,stride,jlshape = ncsub(indexes) | ||||||
| nc_get_vars!(v.ds.ncid,v.varid,start,count,stride,aout) | ||||||
| end | ||||||
|
|
||||||
| function _read_data_from_nc!(v::Variable{T,N}, aout, indexes::Union{Int,Colon,AbstractRange{<:Integer}}...) where {T,N} | ||||||
| sz = size(v) | ||||||
| start,count,stride = ncsub2(sz,indexes...) | ||||||
| jlshape = _shape_after_slice(sz,indexes...) | ||||||
| nc_get_vars!(v.ds.ncid,v.varid,start,count,stride,aout) | ||||||
| end | ||||||
|
|
||||||
| _read_data_from_nc!(v::Variable, aout) = _read_data_from_nc!(v, aout, 1) | ||||||
|
|
||||||
| function writeblock!(v::Variable, data, indexes::TI...) where TI <: Union{AbstractUnitRange,StepRange} | ||||||
| datamode(v.ds) | ||||||
| _write_data_to_nc(v, data, indexes...) | ||||||
| return data | ||||||
| end | ||||||
|
|
||||||
| function _write_data_to_nc(v::Variable{T,N},data,indexes::Int...) where {T,N} | ||||||
| nc_put_var1(v.ds.ncid,v.varid,[i-1 for i in reverse(indexes)],T(data[1])) | ||||||
| end | ||||||
|
|
||||||
| _write_data_to_nc(v::Variable, data) = _write_data_to_nc(v, data, 1) | ||||||
|
|
||||||
| function _write_data_to_nc(v::Variable{T, N}, data, indexes::StepRange{Int,Int}...) where {T, N} | ||||||
| start,count,stride,jlshape = ncsub(indexes) | ||||||
| nc_put_vars(v.ds.ncid,v.varid,start,count,stride,T.(data)) | ||||||
|
There was a problem hiding this comment. Probably DiskArrays.jl wont let you do this optimisation either. You may need to override these methods on Just the But you can also leave this optimisation for later too. There was a problem hiding this comment. I think by defining |
||||||
| end | ||||||
|
|
||||||
| function _write_data_to_nc(v::Variable, data, indexes::Union{AbstractRange{<:Integer}}...) | ||||||
| ind = prod(length.(indexes)) == 1 ? first.(indexes) : normalizeindexes(size(v),indexes) | ||||||
| return _write_data_to_nc(v, data, ind...) | ||||||
| end | ||||||
|
|
||||||
| function grow!(v::CFVariable, data, indexes::Union{Integer, Colon}...) | ||||||
| unlimdims = unlimited(Dimensions(CommonDataModel.dataset(v))) | ||||||
| length(unlimdims) == 1 || error("Only 1 unlimited dimension is supported") | ||||||
| alldims = dimnames(v) | ||||||
| iunlimdim = findfirst(==(unlimdims[1]), alldims) | ||||||
| icol = findfirst(x -> x isa Colon, indexes) | ||||||
|
There was a problem hiding this comment. What if there are other colons? shouldn't you just check that the There was a problem hiding this comment. You're right, I must admit I implemented this pretty rapidly, and I didn't really test it. But I guess we'll have to look more deeply into it given the issue above! There was a problem hiding this comment. As you saw with the bugs in DiskArrays edge cases its actually pretty hard to do it properly. |
||||||
| iunlimdim == icol || ArgumentError("The Colon in the arguments must match the unlimited dimension.") | ||||||
| inds = replace(indexes, Colon() => 1:length(data)) | ||||||
| v[inds...] = data | ||||||
| return v | ||||||
| end | ||||||
|
|
||||||
| getchunksize(v::Variable) = getchunksize(haschunks(v),v) | ||||||
| getchunksize(::DiskArrays.Chunked, v::Variable) = chunking(v)[2] | ||||||
| # getchunksize(::DiskArrays.Unchunked, v::Variable) = DiskArrays.estimate_chunksize(v) | ||||||
| getchunksize(::DiskArrays.Unchunked, v::Variable) = size(v) | ||||||
| eachchunk(v::CFVariable) = eachchunk(v.var) | ||||||
|
There was a problem hiding this comment. These are defined on |
||||||
| haschunks(v::CFVariable) = haschunks(v.var) | ||||||
| eachchunk(v::Variable) = DiskArrays.GridChunks(v, Tuple(getchunksize(v))) | ||||||
| haschunks(v::Variable) = (chunking(v)[1] == :contiguous ? DiskArrays.Unchunked() : DiskArrays.Chunked()) | ||||||
|
|
||||||
| _normalizeindex(n,ind::Base.OneTo) = 1:1:ind.stop | ||||||
| _normalizeindex(n,ind::Colon) = 1:1:n | ||||||
| _normalizeindex(n,ind::Int) = ind:1:ind | ||||||
|
|
@@ -441,74 +429,5 @@ end | |||||
| return start,count,stride | ||||||
| end | ||||||
|
|
||||||
| Base.getindex(v::Union{MFVariable,DeferVariable},ci::CartesianIndices) = v[ci.indices...] | ||||||
| Base.setindex!(v::Union{MFVariable,DeferVariable},data,ci::CartesianIndices) = setindex!(v,data,ci.indices...) | ||||||
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we aiming to move more of these to DiskArrays.jl?
Just having
Variablemeans this PR doesn't allow Rasters.jl integration of gribb or netcdf.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm experimenting with this on this PR: JuliaGeo/CommonDataModel.jl/pull/9.
The issue I'm currently facing is, I think, related to the growth of unlimited dimensions. As you predicted, this causes problems when CFVariable implements DiskArray.
Unfortunately, I can't show you the error stack properly here since it needs the combination of this branch with JuliaGeo/CommonDataModel.jl/pull/9. Is there a way to tell github to run CI with a specific branch of a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a Project.toml and Mainfest.toml to /test if you need too, but its a bit of fiddling and maybe no better than just pasting errors in issues.
Yes that is fairly predictable. You will need to rebuild the chunking after
grow!.