Ref -> Array{Bool,0} in NCDataset to make types concrete

[maximilian-gelbrecht]

In the definition of NCDataset Ref{Bool} isn't a concrete type, but Base.RefValue{Bool} is.

The definition with Ref{Bool} is causing problems for me when using NCDatasets as part of our models with Reactant.jl, and changing it to Base.RefValue{Bool} fixes that.

[Alexander-Barth]

Being compatible with Reactant.jl is indeed something I would like to see. :-)
I am wondering whether Base.RefValue is part of the public API ?
JuliaLang/julia#58101

An alternative could be zero-dimensional arrays:

julia> struct Foo; f::Array{Bool,0}; end

julia> fun(p) = (p.f,p.f[])
fun (generic function with 1 method)

julia> foo = Foo(fill(true))

# type-stable (unlike Ref)
julia> @code_warntype(fun(foo))
MethodInstance for fun(::Foo)
  from fun(p) @ Main REPL[25]:1
Arguments
  #self#::Core.Const(Main.fun)
  p::Foo
Body::Tuple{Array{Bool, 0}, Bool}
1 ─ %1 = Base.getproperty(p, :f)::Array{Bool, 0}
│   %2 = Base.getproperty(p, :f)::Array{Bool, 0}
│   %3 = Base.getindex(%2)::Bool
│   %4 = Core.tuple(%1, %3)::Tuple{Array{Bool, 0}, Bool}
└──      return %4

foo is not a concrete type, but a Base.RefValue is apparently not concrete either:

julia> isconcretetype(foo)
false

julia> r = Ref(true)
Base.RefValue{Bool}(true)

julia> isconcretetype(r)
false

[maximilian-gelbrecht]

Thanks for the quick response!

foo is not a concrete type, but a Base.RefValue is apparently not concrete either:

It is, you just made a small mistake there: isconcretetype takes in types, not instances:

julia> isconcretetype(Base.RefValue{Bool})
true

julia> isconcretetype(Ref{Bool})
false

Being compatible with Reactant.jl is indeed something I would like to see. :-)
I am wondering whether Base.RefValue is part of the public API ?
JuliaLang/julia#58101

Yeah, that's a bit odd in my opinion, because why is the abstract type public API, but the concrete type isn't?
Personally, I wouldn't mind having a Base.RefValue here, but it's your call!

We could also just have a type parameter for the ref value, that would work as well, I think, without using Base.RefValue explicitly, so:

mutable struct NCDataset{TDS,Tmaskingvalue, RefV} <: AbstractNCDataset where TDS <: Union{AbstractNCDataset,Nothing}
    # parent_dataset is nothing for the root dataset
    parentdataset::TDS
    ncid::Cint
    iswritable::Bool
    # true of the NetCDF is in define mode (i.e. metadata can be added, but not data)
    # need to be a reference, so that remains syncronised when copied
    isdefmode::RefV
    # mapping between variables related via the bounds attribute
    # It is only used for read-only datasets to improve performance
    _boundsmap::Dict{String,String}
    maskingvalue::Tmaskingvalue
end

Btw as you seem interested about Reactant compatibility in general: so far I am just starting with this for our model (SpeedyWeather.jl). Without this fix you can't have a NCDataset in a struct that you use in a call to Reactant's compiler, even if you don't end up using it at all. With the fix, I am able to do that. I am not yet at actual output. But Oceananigans uses NCDatasets as well and is Reactant compatible, so they probably already have that figured out. The difference there is just that Oceananigans doesn't include the outputter in their model types like we do in SpeedyWeather.

[Alexander-Barth]

Ah, that makes sense:

julia> isconcretetype(Base.RefValue{Bool})
true

julia> isconcretetype(Array{Bool,0})
true

While Array{Bool,0} seems hacky, it has minimal impact of the API and maintainability. In the future, if we are dropping or reordering type parameters, we break the API of NCDatasets, so I would like to add only type parameter if it is really necessary...
But can you check if that would be fine for Reactant.jl ?

[maximilian-gelbrecht]

Yes, the 0d-array works as well. With Reactant as well, afaik as long as it's concrete it's fine for Reactant.

[Alexander-Barth]

Thanks a lot, Maximilian!