Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

DiskArrays for CFVariable #9

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

tcarion
Copy link
Member

@tcarion tcarion commented Apr 14, 2023

As discussed in issue #8.
It currently requires the implementing modules to extend the Base.parent method for the Variables, that should return the reference to the underlying array. That makes sense for GRIBDatasets, but I don't think it will be that easy for NCDatasets... This PR is needed for GRIBDataset in Raster to work, but simultaneously, it breaks NCDatasets. So consider this PR has a shared experimentation for now :-)

@tcarion tcarion changed the title Make AbstractVariable an AbstractDiskArray DiskArrays for CFVariable Jun 8, 2023
@rafaqz
Copy link
Member

rafaqz commented Jun 8, 2023

Why is it hard to extend Base.parent? Cant you just return the variable itself? Thats what e.g. Array does.

@tcarion
Copy link
Member Author

tcarion commented Jun 8, 2023

This Base.parent issue is not relevant anymore (tbh, I can't remember what the problem was at that time :-) )

@rafaqz
Copy link
Member

rafaqz commented Jan 31, 2025

@Alexander-Barth did you try at this after our last discussion?

(I'm realising GRIBDatasets.jl is a bit broken without this, and the hacks I've done to make it work in Rasters are not holding up well. SO I can work on this PR a bit to get it done, if that would be helpful, and if the plan is to merge it)

@Alexander-Barth
Copy link
Member

Yes, the plan is to merge it :-)
Last time I tried, I ran into issues about zero dimensional arrays.

vv = defVar(ds,"foo",Float64,())
vv[] = 12
ERROR: MethodError: Cannot `convert` an object of type Array{Float64, 0} to an object of type Float64                                                                                                                                
The function `convert` exists, but no method is defined for this combination of argument types.                                                                                                                                      
                                                                                                                                                                                                                                     
Closest candidates are:                                                                                                                                                                                                              
  convert(::Type{T}, ::T) where T<:Number                                                                                                                                                                                            
   @ Base number.jl:6                                                                                                                                                                                                                
  convert(::Type{T}, ::T) where T                                                                                                                                                                                                    
   @ Base Base.jl:126                                                                                                                                                                                                                
  convert(::Type{T}, ::Number) where T<:Number                                                                                                                                                                                       
   @ Base number.jl:7                                                                                                                                                                                                                
  ...                                                                                                                                                                                                                                
                                                                                                                                                                                                                                     
Stacktrace:                                                                                                                                                                                                                          
  [1] setindex!(A::Array{Float64, 0}, x::Array{Float64, 0}, i::Int64)                                                                                                                                                                
    @ Base ./array.jl:987                                                                                                                                                                                                            
  [2] _setindex!                                                                                                                                                                                                                     
    @ ./abstractarray.jl:1436 [inlined]                                                                                                                                                                                              
  [3] setindex!                                                                                                                                                                                                                      
    @ ./abstractarray.jl:1413 [inlined]                                                                                                                                                                                              
  [4] setindex!(::CommonDataModel.MemoryVariable{Float64, 0, MemoryDataset{Nothing, Missing}, Array{Float64, 0}}, ::Array{Float64, 0})                                                                                               
    @ CommonDataModel ~/.julia/dev/CommonDataModel/src/memory_dataset.jl:77                                                                                                                                                          
  [5] writeblock!(::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, ::Array{Float64, 0})                                                                                   
    @ CommonDataModel ~/.julia/dev/CommonDataModel/src/cfvariable.jl:499                                                                                                                                                             
  [6] writeblock_sizecheck!(::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, ::Array{Float64, 0})                                                                         
    @ DiskArrays ~/.julia/packages/DiskArrays/PmkL9/src/diskarray.jl:344                                                                                                                                                             
  [7] setindex_disk_nobatch!(a::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, v::Array{Float64, 0}, i::Tuple{})                                                          
                                                                                                                                                                                                                                     
    @ DiskArrays ~/.julia/packages/DiskArrays/PmkL9/src/diskarray.jl:316                                                                                                                                                             
  [8] setindex_disk!(::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, ::Array{Float64, 0})                                                                                
    @ DiskArrays ~/.julia/packages/DiskArrays/PmkL9/src/diskarray.jl:352                                                                                                                                                             
  [9] setindex!(::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, ::Array{Float64, 0})                                                                                     
    @ CommonDataModel ~/.julia/packages/DiskArrays/PmkL9/src/diskarray.jl:386                                                                                                                                                        
 [10] setindex!(::CommonDataModel.CFVariable{Float64, 0, CommonDataModel.MemoryVariable{…}, Attributes{…}, @NamedTuple{…}}, ::Float64)                                                                                               
    @ CommonDataModel ~/.julia/packages/DiskArrays/PmkL9/src/diskarray.jl:390                                                                                                                                                        
 [11] top-level scope                                                                                                                                                                                                                
    @ REPL[57]:1                                                                                                                                                                                                                     
Some type information was truncated. Use `show(err)` to see complete types.              

Ideally, it should work like this:

a = Array{Float64,0}(undef,())                                                                                                                                 
# 0-dimensional Array{Float64, 0}:                                                                                                                                      
# 6.90859146202467e-310                                                                                                                                                 
a[] = 123                                                                                                                                                      

I will need to investigate what the problem is. Maybe I need a special case for 0-dimensional arrays and empty indices, similar to this:

https://github.com/JuliaGeo/NCDatasets.jl/blob/c9004bfceb7a2078047647d0703998623b9dc820/src/variable.jl#L427

@rafaqz
Copy link
Member

rafaqz commented Feb 9, 2025

That looks like your setindex! doesn't handle writing zero dimensional arrays to zero dimensional arrays.

But we will want to skip around DiskArrays indexing completely for the memory backed arrays so this won't be hit anyway. That just means adding the same getindex/setindex etc that DiskArrays adds, but on the specific variable type. But it could also be done with a simple trait if we want to add it to DiskArrays.

But good to know the intention is to merge the PR!

I think we will need to delete a lot of code here for it to work consistently. The SubArray implementation especially. As shown in the NCDatasets.jl issue deleting those methods is whats needed to make broadcasts work. CatArrays.jl can also be deleted for the same reason.

I started on that but I'm just not sure what degree of changes will be accepted, so it's hard for me to be motivated to put in the time needed to do it.

My current thinking is its easier for me to insulate Rasters from the bugs in CommonDataModel variables by always wrapping variables in a proper DiskArray that we can be sure works.

But long term for the ecosystem we should fix CDM/DiskArrays interop

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants