Make AbstractVariable a subtype of AbstractDiskArray #35

lupemba · 2025-05-03T14:01:13Z

This PR requires JuliaIO/DiskArrays.jl#255 and JuliaIO/DiskArrays.jl#260

See #32 for background.

This is a draft of how we can make AbstractVariable <: AbstractDiskArray plus how to use the AbstractSubDiskArray for views.

This draft does not address:

Performance
(solved) Documentation
PermutedDiskArray

This PR will probably contain a few breaking changes but most things will behave identically. One notable change is that SubVariable is not longer a subtype of AbstractVariable.

This update will requirer packages that implement the CommonDataModel interface to stop defining getindex and setindex. Instead they should define DiskArrays.readblock!and DiskArrays.writeblock! which most of them already do.

Project.toml

src/CommonDataModel.jl

src/cfvariable.jl

lupemba · 2025-05-03T14:06:20Z

src/groupby.jl

@@ -462,6 +462,9 @@ Base.BroadcastStyle(::Type{<:ReducedGroupedVariable}) = ReducedGroupedVariableSt
 Base.BroadcastStyle(::DefaultArrayStyle,::ReducedGroupedVariableStyle) = ReducedGroupedVariableStyle()
 Base.BroadcastStyle(::ReducedGroupedVariableStyle,::DefaultArrayStyle) = ReducedGroupedVariableStyle()

+Base.BroadcastStyle(::DiskArrays.ChunkStyle,::ReducedGroupedVariableStyle) = ReducedGroupedVariableStyle()


ReducedGroupedVariable use different broadcasting than DiskArrays. This might lead to some confusion.

src/memory_dataset.jl

lupemba · 2025-05-03T14:10:20Z

src/variable.jl

@@ -203,7 +203,7 @@ Defines and return the variable in the data set `ds`
 copied from the variable `src`. The dimension name, attributes
 and data are copied from `src` as well as the variable name (unless provide by `name`).
 """
-function defVar(dest::AbstractDataset,varname::SymbolOrString,srcvar::AbstractVariable; kwargs...)
+function defVar(dest::AbstractDataset,varname::SymbolOrString,srcvar::Union{AbstractVariable, SubVariable}; kwargs...)


I updated ::AbstractVariable to ::Union{AbstractVariable, SubVariable} the places I thought it made sense but I might have missed some.

Isn't SubVariable <: AbstractVariable anyway ?

yes, this is indeed the case

CommonDataModel.jl/src/types.jl

Line 158 in 68f2050

struct SubVariable{T,N,TA,TI,TAttrib,TV} <: AbstractVariable{T,N}

No, see the changes to types.jl

https://github.com/JuliaGeo/CommonDataModel.jl/pull/35/files#diff-525588e68b2421901be164965272940effa093de733a3fd36c4b9a4344b8c20cR55

SubVariable is no longer a subtype of AbstractVariable but is now a subtype of AbstractSubDiskArray

struct SubVariable{T,N,P,I,L} <: DiskArrays.AbstractSubDiskArray{T,N,P,I,L}

This is done to use the DiskArray implementation of views as discussed in JuliaGeo/NCDatasets.jl#274

Ah of course

I have add ::Union{AbstractVariable, SubVariable} all the places needed to extend the following functions for SubVariable: filter, coord, ancillaryvariables, groupby, select. I think that should cover the public interface of CommonDataModel

lupemba · 2025-05-03T14:11:00Z

src/variable.jl

@@ -310,6 +310,20 @@ function Base.show(io::IO,v::AbstractVariable)
 end


+function DiskArrays.haschunks(v::AbstractVariable) 
+    storage, chunksizes = chunking(v) 


I tried reuse the exiting chunking method.

test/test_groupby.jl

test/test_scaling.jl

test/test_subvariable.jl

lupemba · 2025-05-03T14:15:33Z

@rafaqz @tiemvanderdeure @Alexander-Barth,
Here is my work so far. The tests passes when I add the DiskArrays changes from JuliaIO/DiskArrays.jl#255 and JuliaIO/DiskArrays.jl#260

rafaqz · 2025-05-03T18:33:15Z

Great, we should get those DiskArrays PRs in then (sorry got distracted with other things)

tiemvanderdeure · 2025-05-05T07:03:45Z

Yeah we just need JuliaIO/DiskArrays.jl#249 and then we can merge JuliaIO/DiskArrays.jl#255. I think both are pretty much ready to merge?

rafaqz · 2025-05-05T08:50:28Z

I just need to fix views in 249

lupemba · 2025-06-23T09:40:19Z

src/groupby.jl

+    aout,
+    indexes::Vararg{OrdinalRange, N}) where {T,N}
+
+    aout .= Base.getindex(gr,indexes...)


Currently Base.getindex is defined for ReducedGroupedVariable so the DiskArray.jl implementation is not used. An alternative would be to use the DiskArray getindex and implement more logic in readblock!. This is more ReducedGroupedVariable and I can not see any benefit of doing it right now.

lupemba · 2025-06-27T12:32:10Z

@rafaqz @tiemvanderdeure @Alexander-Barth,

This PR is now ready for review. There is still some unresolved comments that we will have to discuss before merging the PR.
I have made draft PRs to all the "datasets" packages to test that the this update will work.

Most of the changes are small but TIFFDatasets required a bit more work since it was not using DiskArrays.

I think the path forward should be.

Review this PR and solve outstanding comments.
Do some minor performance tests
Merge this PR and release CommonDataModel.jl v0.4
Review NCDatasets.jl PR and rerun CI.
Release new NCDatasets.jl version. Some of the other packages uses NCDatasets as a test dependency.
Update the remaining packages.

lupemba · 2025-06-27T12:33:45Z

The CI / Documentation is failing because https://psl.noaa.gov/thredds/fileServer/Datasets/noaa.oisst.v2.highres/sst.day.mean.2023.nc is unavailable at the moment.

src/multifile.jl

felixcremer · 2025-08-11T09:32:16Z

Is this PR also closing #8 or is there anything else left for full DiskArrays compatibility?
I guess this also supersedes #9 as a way to achieve the same goal.

Is there something I could help with pushing this forward?

…issues in docs).

Replace if N==0 with method and use dispatch.

lupemba · 2025-08-11T19:31:30Z

Hi, I just did some minor adjustments and added some more tests.
I think that the PR is now in a good condition and we should be able to move forward. The next step will be to get an approval from @Alexander-Barth, squash merge the PR and then make a new breaking release. Once it is done we can update all the related PRs for the dataset packages and handle any bugs that might pop up. The majority of users will only be affected once we release the new dataset packages.

Note that the Documentation works with JuliaGeo/NCDatasets.jl#279

Is there something I could help with pushing this forward?

@felixcremer, some performance tests would be good. I haven't really done any yet. There are some tests defined in commondatamodel/test/perf but they require linux and sudo access. Also suggestions for extra tests to increase code coverage is welcome.

felixcremer · 2025-08-13T13:07:26Z

I just started playing around with it in Rasters and this will also need some overhaul in Rasters.jl to bring it to the new behaviour.

rafaqz · 2025-08-13T23:33:56Z

Yeah Rasters had to hack around the old behavior a bit to get at the inner disk arrays, maybe some of that will break.

tiemvanderdeure · 2025-08-14T05:52:16Z

Probably we can just revert rafaqz/Rasters.jl#892 once all of this is working

Alexander-Barth · 2025-08-27T18:50:51Z

This looks really good ! Thank you @lupemba !!!

What kind of breaking changes do you expect (besides SubVariable is not longer a subtype of AbstractVariable) ?

lupemba · 2025-08-27T19:36:14Z

What kind of breaking changes do you expect (besides SubVariable is not longer a subtype of AbstractVariable) ?

@Alexander-Barth, I am glad you like the PR 😄 The main breaking change is that packages implementing CommonDataModel.jl have to define DiskArray.readblock! and DiskArray.writeblock! instead of Base.getindex and Base.setindex! when implementing a new AbstractVariable. This PR to ZarrDatasets.jl is a good example https://github.com/JuliaGeo/ZarrDatasets.jl/pull/13/files

This is the only other breaking change that I am aware of. The effect on the users of the ***Datasets.jl packages should be very minimal. I therefore think we can get away with a non breaking updates to the ***Datasets.jl packages.

lupemba · 2025-08-31T14:19:47Z

@Alexander-Barth and @rafaqz
It would be nice if you could approve this PR or let me know if anything more needs to be change. I would like to merge this within the next week so it doesn't drag out forever and go stale.

rafaqz · 2025-09-01T05:33:59Z

src/memory_dataset.jl

+
+    root = _root(v)
+    for idim = findall(size(v) .> sz)
+        dname = v.dimnames[idim]


Suggested change

dname = v.dimnames[idim]

dname = dimnames(v)[idim]

There is a bunch of direct field access in this PR for fields that have interface methods

Yes, this code is simply copied from the Base.setindex! method that was before. I think the best approach is to open a separate issue and PR to reduce "direct field access". This can be looked at after this PR is completed.
#42

I don't think we should expand the scope of this PR anymore.

Sure, it just looks like new code to me and you use assessor methods everywhere else...

Make AbstractVariable a subtype of AbstractDiskArray

81f24cd

lupemba commented May 3, 2025

View reviewed changes

felixcremer marked this pull request as ready for review May 15, 2025 16:36

lupemba mentioned this pull request May 28, 2025

add abstract types for views and permuted arrays JuliaIO/DiskArrays.jl#255

Merged

lupemba added 2 commits June 19, 2025 19:08

Bump DiskArrays

a30bf71

Bump version and minor fixes based on NCDatasets tests

2e06265

lupemba mentioned this pull request Jun 22, 2025

Prepare common data model 0.4 JuliaGeo/NCDatasets.jl#279

Draft

lupemba added 2 commits June 23, 2025 10:32

Forward readblock! and writeblock! CFVariable

7c382b8

Replace DiskArrays._replace_colon with two copied lines.

0321c61

lupemba marked this pull request as draft June 23, 2025 08:55

Implement DiskArrays.readblock! for ReducedGroupedVariable

c6cb2f0

lupemba commented Jun 23, 2025

View reviewed changes

Update documentation and add MetopDatasets.jl to integration tests

f9df8eb

lupemba marked this pull request as ready for review June 27, 2025 12:20

tiemvanderdeure mentioned this pull request Jul 8, 2025

DiskIndex is not inferred JuliaIO/DiskArrays.jl#266

Closed

Alexander-Barth and others added 5 commits August 9, 2025 18:33

add Base.propertynames (issue JuliaGeo#34)

1edfc96

remove trailing white-space

7232d96

GLMakie is not used

1472682

fix version numbers for documentation

5159cb7

Broken test works with new DiskArrays.

e9f6418

Clean up N==0 in writeblock! and replace assert with error.

ef58671

lupemba commented Aug 9, 2025

View reviewed changes

src/multifile.jl Outdated Show resolved Hide resolved

lupemba added 2 commits August 11, 2025 15:31

Bump DiskArrays and add SubVariable to more method signatures (Fixes …

f47f24b

…issues in docs).

Add additional test to increase diff code coverage.

a03812e

Replace if N==0 with method and use dispatch.

felixcremer mentioned this pull request Aug 13, 2025

Raster from sub Zarr group fails on getindex rafaqz/Rasters.jl#1001

Open

Merge branch 'main' into subtype-AbstractDiskArray

5d1a5e8

rafaqz approved these changes Sep 1, 2025

View reviewed changes

rafaqz reviewed Sep 1, 2025

View reviewed changes

Make AbstractVariable a subtype of AbstractDiskArray #35

Are you sure you want to change the base?

Make AbstractVariable a subtype of AbstractDiskArray #35

Conversation

lupemba commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lupemba commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaqz commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiemvanderdeure commented May 5, 2025

Uh oh!

rafaqz commented May 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lupemba commented Jun 27, 2025

Uh oh!

lupemba commented Jun 27, 2025

Uh oh!

Uh oh!

felixcremer commented Aug 11, 2025

Uh oh!

lupemba commented Aug 11, 2025

Uh oh!

felixcremer commented Aug 13, 2025

Uh oh!

rafaqz commented Aug 13, 2025

Uh oh!

tiemvanderdeure commented Aug 14, 2025

Uh oh!

Alexander-Barth commented Aug 27, 2025

Uh oh!

lupemba commented Aug 27, 2025

Uh oh!

lupemba commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lupemba Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lupemba commented May 3, 2025 •

edited

Loading

lupemba commented May 3, 2025 •

edited

Loading

rafaqz commented May 3, 2025 •

edited

Loading

lupemba Sep 1, 2025 •

edited

Loading