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

Convenience 'parameters' field in Model for user-defined functions #394

Closed
glwagner opened this issue Sep 10, 2019 · 3 comments · Fixed by #395
Closed

Convenience 'parameters' field in Model for user-defined functions #394

glwagner opened this issue Sep 10, 2019 · 3 comments · Fixed by #395
Labels
feature 🌟 Something new and shiny

Comments

@glwagner
Copy link
Member

I wonder if it may be a good idea to add a generic, user-definable 'parameters' field in Model.

The main purpose of this field would be to provide the user with more flexibility for boundary condition and forcing functions. Consider the implementation of a simple sponge layer in a script:

const dTdz = 0.01
const μ₀ = 0.02

@inline μ(z, Lz) = μ₀ * exp(-(z + Lz) / 0.05Lz)
@inline T₀(z) = dTdz * z

@inline FT(grid, U, Φ, i, j, k) = @inbounds -μ(grid.zC[k], grid.Lz) *.T[i, j, k] - T₀(grid.zC[k]))

model = Model(forcing=Forcing(FT=FT), ...)

This works, of course. However, it requires the global consts dTdz and μ₀ in order to compile on the GPU. This prevents users from, for example, defining a function of the form create_and_run_model(μ₀, dTdz, other_parameters...). The only way to set μ₀ or dTdz is by defining them as global consts; therefore a new script (or argument parsing via ArgParse and bash scripting) is required for each new run.

I feel this is a potentially major limitation to current and future automation and we should attempt to find a solution to address the problem.

One partial solution is to add a field parameters to the model. This field is then passed down into calculate_interior_source_terms! and the boundary condition algorithm to be used, if desired, within a user-defined functions. In this case, the above pattern could become:

@inline μ(z, Lz, μ₀) = μ₀ * exp(-(z + Lz) / 0.05Lz)
@inline T₀(z, dTdz) = dTdz * z

@inline FT(grid, U, Φ, i, j, k, params) = @inbounds -μ(grid.zC[k], grid.Lz, params.μ₀) *.T[i, j, k] - T₀(grid.zC[k], params.dTdz))

model = Model(forcing = Forcing(FT=FT), parameters=(μ₀=0.02, dTdz=0.01), ...)

We need not restrict the type of the field parameters; we can simply add

mutable struct Model{P, ...}
    ...
    parameters :: P
end

And set it to nothing by default in the Model constructor. The user may provide any kind of parameters they wish.

@ali-ramadhan ali-ramadhan added the feature 🌟 Something new and shiny label Sep 10, 2019
@ali-ramadhan
Copy link
Member

I like this! I do agree about the current setup limiting future automation and reproducibility.

So I guess the idea is that params is completely user-defined and is available to forcing functions and boundary conditions. So it shouldn't be used by the Model itself.

@glwagner
Copy link
Member Author

Right --- I think any required fields would either complicate it's specification or limit how it could be used, so I think it makes sense that it's either user-defined, or nothing.

I'll go ahead an open a PR if we think this is a good idea (and that parameters is a good name).

@ali-ramadhan
Copy link
Member

Sounds good! Doesn't interfere with current code so should be an easy merge.

I can update benchmark_forcing_functions.jl with a benchmark that uses params to make sure it isn't slowing things down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 Something new and shiny
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants