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

Abstraction, type-hierarchy and modularity #222

Closed
milankl opened this issue Jan 9, 2023 · 0 comments · Fixed by #217
Closed

Abstraction, type-hierarchy and modularity #222

milankl opened this issue Jan 9, 2023 · 0 comments · Fixed by #217
Labels
structure 🏠 Internal structure of the code style 💄 Coding but in style

Comments

@milankl
Copy link
Member

milankl commented Jan 9, 2023

I've started #217 which was motivated as I realised it is better to have the Parameters struct parametric with the model, i.e. Parameters{Model<:ModelSetup}. TL;DR please don't use model anymore as keyword argument, instead:

run_speedy(NF,SomeAbstractModel;kwargs...)    # or
run_speedy(NF;kwargs...)                      # or
run_speedy(SomeAbstractModel;kwargs...)

where NF<:AbstractFloat (as before) and SomeAbstractModel is an abstract type branching off the model hierarchy, following this logic:

  • AbstractSupertype A
    • AbstractSubtype B
      • ConcreteType C
    • (potential) OtherAbstractSubtype B2
      • (potential) OtherConreteType C2

With most functions defined for f(::A) (the most abstract type). This is important for modularity as we want to be able to extend without redefining what doesn't need to be redefined: If we implement f(::A), then anyone can define B2,C2 later and all functions still work, only those where things differ can then be defined as f(::B2).

Longer version: These last two sentences ☝🏼 are very important, and unfortunately, we (particularly me) haven't been very consistent on this. Applied to our model hierarchy, we currently have

  • (abstract) ModelSetup
    • (abstract) Barotropic
      • (concrete) BarotropicModel
    • (abstract) ShallowWater
      • (concrete) ShallowWaterModel
    • (abstract PrimitiveEquation
      • (abstract) PrimitiveDryCore
        • (concrete) PrimitiveDryCoreModel
      • (abstract) PrimitiveWetCore
        • (concrete) PrimitiveWetCoreModel

So we should in general define

  • f(::ModelSetup) so that f applies to all models
  • unless f doesn't make sense for a given group of models, in that case define something like f(::Barotropic) (important: Barotropic is an abstract type, not the concrete BarotropicModel as someone may want to define another model <:Barotropic in the future and then can reuse (probably most functions like) f(::Barotropic).

@maximilian-gelbrecht @white-alistair @dmey

@milankl milankl added style 💄 Coding but in style structure 🏠 Internal structure of the code labels Jan 9, 2023
@milankl milankl added this to the v0.5 milestone Jan 9, 2023
@milankl milankl linked a pull request Jan 9, 2023 that will close this issue
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 style 💄 Coding but in style
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant