Skip to content
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

set_vars! function to easily set variables in PrognosticVariables #154

Merged
merged 6 commits into from
Sep 29, 2022
Merged

set_vars! function to easily set variables in PrognosticVariables #154

merged 6 commits into from
Sep 29, 2022

Conversation

maximilian-gelbrecht
Copy link
Member

@maximilian-gelbrecht maximilian-gelbrecht commented Sep 27, 2022

This is a currently a draft, I want to get a review on it before I fully finish it.

  • I implemented set....!(prong,...) functions for all variables for spectral, grid and matrix input. These inputs are currently all Vector{...}

  • no doc strings yet

  • initializiation routines could be reworked with this as well

  • maybe there are some other parts of the code that could use this @milankl ?

I was also thinking of implementing the opposing get....(progn...) functions that return the Vectors{...}. What do you think?

A small observation: for the ModelSetup there should probably be a has(M::ModelSetup, var_name::Symbol) function, that could make a small minor things a bit more transparent.

@maximilian-gelbrecht maximilian-gelbrecht added the array types 🌐 LowerTriangularMatrices and RingGrids label Sep 27, 2022
@maximilian-gelbrecht maximilian-gelbrecht changed the title initial var_args commit Set_vars function to easily set variables in PrognosticVariables Sep 27, 2022
@maximilian-gelbrecht maximilian-gelbrecht changed the title Set_vars function to easily set variables in PrognosticVariables set_vars! function to easily set variables in PrognosticVariables Sep 27, 2022
@milankl
Copy link
Member

milankl commented Sep 27, 2022

* I implemented `set....!(prong,...)` functions for all variables for spectral, grid and matrix input. These inputs are currently all `Vector{...}`

Maybe we could even add methods like set_vorticity!(progn,L,lf=1) such that for a L being a LowerTriangularMatrix, or Matrix, or Gridded, (and not a vector of it) one can easily set the right leapfrog time step lf or layer layer etc.

* initializiation routines could be reworked with this as well
* maybe there are some other parts of the code that could use this @milankl ?

I think only the initial conditions actually set_...! the prognostic variables.

I was also thinking of implementing the opposing get....(progn...) functions that return the Vectors{...}. What do you think?

Yes, the time integration has lines like

vor_old  = progn.leapfrog[1].vor
vor_new  = progn.leapfrog[2].vor

which could be wrapped more generally into vor_old, vor_new = get_vorticity(progn,lf=(1,2)) but maybe it's not worth it?

A small observation: for the ModelSetup there should probably be a has(M::ModelSetup, var_name::Symbol) function, that could make a small minor things a bit more transparent.

Yes, at the moment, we set up all prognostic variables as a dummy 0-element array when they are not used in a given ModelSetup. E.g. in the BaroptropicModel

julia> p.layers[1].leapfrog[1].div
0×0 LowerTriangularMatrix{ComplexF64}

So if it's useful for you please implement has(M::ModelSetup, var_name::Symbol). At the moment we have lines like

M isa BarotropicModel ? nothing : gridded!(diagn.surface.pres_grid,progn.pres.leapfrog[lf],S)

which could then take the form like

has(M,:pres) ? nothing : gridded!(diagn.surface.pres_grid,progn.pres.leapfrog[lf],S)

I find both readable, but your suggestions externalises that property of M to a has function which we can define in models.jl to allow them to be more easily extended? I like that.

src/prognostic_variables.jl Outdated Show resolved Hide resolved
Comment on lines 104 to 105
var::Vector{<:LowerTriangularMatrix},
lf::Integer=1) where NF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these lines are changed to

                 var::LowerTriangularMatrix...;
                 lf::Integer=1) where NF

then both L and (L1,L2) could be used as input? (as ... just unravels the tuple?) Is that helpful or would that just complicate things further down?

src/prognostic_variables.jl Show resolved Hide resolved
src/prognostic_variables.jl Outdated Show resolved Hide resolved
src/prognostic_variables.jl Show resolved Hide resolved
src/prognostic_variables.jl Outdated Show resolved Hide resolved
@maximilian-gelbrecht
Copy link
Member Author

I would not add functions/functionality to set individual layers. It does make the functions more complicated (or needs a separate set of functions). I don't see a super important purpose for it, and with the get... functions it is also very easy to emulate that behaviour

vor = get_vorticity(progn)
vor[3] = ...
set_vorticity!(progn, vor) 

@maximilian-gelbrecht
Copy link
Member Author

From my side this is finished.
One important change: I made PrognosticVariables parametric on the ModelSetup. If no ModelSetup is given, then it defaults to the abstract type. I've done this for the has function, so that one can easily return which variables are stored within an instance of PrognosticVariables. Maybe this could also be useful for other stuff in the future.

Comment on lines +80 to +83
for var_name in var_names
if has(progn_new, var_name)
var = get_var(progn_old, var_name)
set_var!(progn_new, var_name, var)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. I like it.

@@ -20,6 +20,8 @@ struct BarotropicModel{NF<:AbstractFloat, D<:AbstractDevice} <: ModelSetup{D}
device_setup::DeviceSetup{D}
end

has(::Type{BarotropicModel{NF,D}}, var_name::Symbol) where {NF,D} = var_name in [:vor]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the ,D} can be dropped as it's not used (I had to remind myself what D stands for again...)? As far as I understand multi-parametric types in Julia they work like type{T1,T2,T3,T4} to be like type{T1}{T2}{T3}

julia> struct ABC{A,B,C}
           a::Int
           b::Int
       end

julia> abc = ABC{Float64,Float32,Float16}(1,2)
ABC{Float64, Float32, Float16}(1, 2)

julia> t(abc::ABC{T}) where T = T(abc.a)
julia> t(abc)
1.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So dispatch is only performed following the first type if the others are not specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why exactly, but it is required, otherwise it won't work. I did try it before. Just again:

 MethodError: no method matching has(::Type{SpeedyWeather.PrimitiveEquationModel{Float64, SpeedyWeather.CPUDevice}}, ::Symbol)
  Closest candidates are:
    has(::Type{SpeedyWeather.BarotropicModel{NF}}, ::Symbol) where NF at ~/Nextcloud/SpeedyFork/SpeedyWeather.jl/src/models.jl:23
    has(::Type{SpeedyWeather.ShallowWaterModel{NF}}, ::Symbol) where NF at ~/Nextcloud/SpeedyFork/SpeedyWeather.jl/src/models.jl:46
    has(::Type{SpeedyWeather.PrimitiveEquationModel{NF}}, ::Symbol) where NF at ~/Nextcloud/SpeedyFork/SpeedyWeather.jl/src/models.jl:71

src/models.jl Show resolved Hide resolved
src/models.jl Show resolved Hide resolved
src/models.jl Show resolved Hide resolved
src/prognostic_variables.jl Show resolved Hide resolved
Comment on lines +116 to +117
@assert length(var) == length(progn.layers)
@assert has(progn, varname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to self that we should do something about error messages in general. I like the @assert here as they actually print what's wrong. However, often I ended up using throw(BoundsError) which is kind of clear if there's only one @boundscheck before a loop in a function but if there's several ones, you don't know which array sizes don't match. Maybe we can make them somehow more telling...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that before, but @assert let's you specify error messages:
@assert has(progn, varname) "No variable with this name found."

@milankl
Copy link
Member

milankl commented Sep 28, 2022

Last question, while we are at it: Should we also include set_vorticity!(progn,0) to just set the vorticity to zero (or any other scalar? This could be done via

function set_var!(progn::PrognosticVariables{NF}, 
                  varname::Symbol, 
                  s::Number;
                  lf::Integer=1) where NF

    for progn_layer in zip(progn.layers
        fill!(getfield(progn_layer.leapfrog[lf], varname), s)
    end 

    return progn 
end 

functions like set_vorticity! should just pass on s::Number etc?

@maximilian-gelbrecht
Copy link
Member Author

Last question, while we are at it: Should we also include set_vorticity!(progn,0) to just set the vorticity to zero (or any other scalar? This could be done via

function set_var!(progn::PrognosticVariables{NF}, 
                  varname::Symbol, 
                  s::Number;
                  lf::Integer=1) where NF

    for progn_layer in zip(progn.layers
        fill!(getfield(progn_layer.leapfrog[lf], varname), s)
    end 

    return progn 
end 

functions like set_vorticity! should just pass on s::Number etc?

Yeah, why not, I'll add it

@maximilian-gelbrecht maximilian-gelbrecht merged commit 6f8e186 into SpeedyWeather:main Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array types 🌐 LowerTriangularMatrices and RingGrids
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants