-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update to Diskarrays 0.4 #247
Update to Diskarrays 0.4 #247
Conversation
Thanks a lot Fabian for your help! |
Can we re-run CI and see if this passes now? |
…sError rather than a generic NCDatasets.NetCDFError
There was a failure when accessing a variable out-of-bounds. Previously test was just checking for an NetCDFError now it is also checking for a BoundsError. The test code is updated here: Currently, we have a failing test for an array of strings and strided writes using Test
using NCDatasets
sz = (4,5)
filename = tempname()
ds = NCDataset(filename,"c")
# define the dimension "lon" and "lat"
ds.dim["lon"] = sz[1]
ds.dim["lat"] = sz[2]
T = String
data = [Char(i+64) * Char(j+64) for i = 1:sz[1], j = 1:sz[2]]
v = defVar(ds,"var-$T",T,("lon","lat"))
v[:,:] = data[:,:] # ok
# stridded write and read
v[1:2:end,1:2:end] = data[1:2:end,1:2:end]
# @test all(v[1:2:end,1:2:end] .== data[1:2:end,1:2:end])
close(ds) We do not have a failure for non-strided writes. The data given to
Do you have any ideas? |
@meggart ? |
Bump, this would be pretty useful to have nowadays since I'm looking at Zarr + raster ecosystem improvements |
I am currently hitting this error with DiskArrays 0.4.4 and NCDatasets 2d0e7cb:
Reported also here: |
Ok, I tested this again with DiskArrays 0.4.5. It looks like tests are passing now after these changes:
|
I just removed the WIP from the PR title. I think all issues with the DiskArrays update should be addressed. |
I have no idea about the failing appveyor test. Which platform is this testing? |
thanks a lot Fabian looking into this! The failing appveyor is on Windows 32-bit (the error also occurs on linux 32-bit). Given the stack-trace, it might be related to
As far I understand the problem, in the test case the array is indexed with an index of the type https://github.com/meggart/DiskArrays.jl/blob/main/src/diskarray.jl#L129 But the type https://github.com/meggart/DiskArrays.jl/blob/main/src/diskarray.jl#L77-L83 (Maybe a similar error can also be triggered on 64-bit platforms using non-Int64 indices, but I am not sure) When I define the following method, then the test passes: import DiskArrays: DiskIndex
DiskIndex(output_size::NTuple{N,<:Integer},temparray_size::NTuple{M,<:Integer},
output_indices::Tuple,temparray_indices::Tuple,data_indices::Tuple) where {N, M} =
DiskIndex(Int.(output_size),Int.(temparray_size),output_indices,temparray_indices,data_indices) Can this be added to DiskArrays? (I can make a PR :-) ) |
Yes, this would be great to have in DiskArrays! |
great @meggart ! All tests now also work on 32-bit julia on linux. When you have the time, can you make a new release of DiskArrays, so I can merge this PR? |
A new version of DiskArrays is tagged, so you can try to rerun CI |
Great, all test do pass! |
This is a placeholder PR to update the DiskArray interface implementation once DiskArrays 0.4 is released.