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

Use BarotropicModel instead of :barotropic etc #176

Merged
merged 5 commits into from
Nov 3, 2022
Merged

Conversation

milankl
Copy link
Member

@milankl milankl commented Nov 3, 2022

Same for ShallowWaterModel, PrimitiveEquationModel.

@milankl
Copy link
Member Author

milankl commented Nov 3, 2022

@maximilian-gelbrecht I thought it would be more consistent to use model::Type{<:ModelSetup} throughout, meaning that we do

run_speedy(model=ShallowWaterModel)

instead of model=:shallowwater. This might give us more scope in the future for externally defined ModelSetups and (most important ofc) it works with auto-completion 😉

@milankl
Copy link
Member Author

milankl commented Nov 3, 2022

Ah same problem as always, we cannot define Parameters without having defined BarotropicModel and vice versa 💩

@maximilian-gelbrecht
Copy link
Member

But here we have an abstract type. Shouldn't this just work when defining abstract type ModelSetup{D} end e.g. directly in the SpeedyWeather.jl file (or somewhere else where it is loaded before all other files).

@milankl
Copy link
Member Author

milankl commented Nov 3, 2022

Yeah, but we then also would need a AbstractBarotropicModel... which can be defined before Parameters and used therein

@maximilian-gelbrecht
Copy link
Member

maximilian-gelbrecht commented Nov 3, 2022

Yeah, use in Parameters wouldn't be a problem, one could do the default argument also by calling a function at run time. But you are also using the model types in define_diffusion.jl which always needs to be called before defining the concrete model types.

Mh, ... so either introducing all the abstract types, or maybe we just keep it as it is?

Long-term, an alternative could also be to do something a bit more akin of the syntax of DiffEq.jl etc:

model = ShallowWaterModel(kwargs...)
p, d = solve(model)

Edit: when I think about it, this also wouldn't solve this issue, it's basically the same...

@@ -86,14 +86,14 @@ function HorizontalDiffusion( P::Parameters, # Parameter struct
end
end

if P.model == :barotropic || P.model == :shallowwater # orographic correction not needed
if P.model <: BarotropicModel || P.model <: ShallowWaterModel # orographic correction not needed

Choose a reason for hiding this comment

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

I'd assume that use of the model types here, would always require the added abstract types. This file needs to be called before the file that defines the models, as it needs HorizontalDiffusion

@milankl
Copy link
Member Author

milankl commented Nov 3, 2022

What about

abstract type ModelSetup{D} end
abstract type Barotropic{D} <: ModelSetup end
abstract type ShallowWater{D} <: ModelSetup end
abstract type PrimitiveEquation{D} <: ModelSetup end

and then subtype BarotropicModel etc from there?

@maximilian-gelbrecht
Copy link
Member

My impression is that other libraries also use more parametric types in the struct because of this problem, and specify the types of the fields in the structs less often. This does definitely come as the cost of less readable code though

@maximilian-gelbrecht
Copy link
Member

What about

abstract type ModelSetup{D} end
abstract type Barotropic{D} <: ModelSetup end
abstract type ShallowWater{D} <: ModelSetup end
abstract type PrimitiveEquation{D} <: ModelSetup end

and then subtype BarotropicModel etc from there?

Yeah, I guess that's the easiest way to do it.

@milankl milankl merged commit fd963f4 into main Nov 3, 2022
@milankl
Copy link
Member Author

milankl commented Nov 3, 2022

Okay that's how it's now!

abstract type ModelSetup end
abstract type Barotropic <: ModelSetup end
abstract type ShallowWater <: ModelSetup end
abstract type PrimitiveEquation <: ModelSetup end

and

struct BarotropicModel <: Barotropic
struct ShallowWaterModel <: ShallowWater
struct PrimitiveEquationModel <: PrimitiveEquation

so without Model it's the abstract supertype that can be used early on, with Model it's the concrete type.

@milankl milankl added user interface 🎹 How users use our user interface structure 🏠 Internal structure of the code labels Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
structure 🏠 Internal structure of the code user interface 🎹 How users use our user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants