Type system is overcomplicated

[glwagner]

I'm wondering if the code is more complicated than it needs to be. There are three types for each universal function, eg BusingerParams, BusingerType, and then Businger.

The "*Type" is superfluous with the *Params types, since each Type is simply related to each Params adding no additional information:

SurfaceFluxes.jl/src/UniversalFunctions.jl

Lines 1113 to 1118 in f83dc86

	universal_func_type(::Type{T}) where {T <: BusingerParams} = BusingerType()
	universal_func_type(::Type{T}) where {T <: GryanikParams} = GryanikType()
	universal_func_type(::Type{T}) where {T <: GrachevParams} = GrachevType()
	universal_func_type(::Type{T}) where {T <: BeljaarsParams} = Beljaars()
	universal_func_type(::Type{T}) where {T <: ChengParams} = ChengType()
	universal_func_type(::Type{T}) where {T <: HoltslagParams} = HoltslagType()

I propose refactoring the code to simplify the type structure by combining the concepts of "params" and "type".

I also cannot see the purpose of the types which are currently called Businger, Gryanik, etc. These types are distinct from the "params" and "type" objects by providing a place to store the Monin-Obukhov length. But these objects to not compute this length; it's computed externally. And the length stored there is never actually used outside of the scope where it is computed. In other words, the only time we access the length L is a few lines after it is computed (here, L_MO is an input argument to the function):

SurfaceFluxes.jl/src/SurfaceFluxes.jl

Lines 806 to 817 in f83dc86

	von_karman_const::FT = SFP.von_karman_const(param_set)
	uf = UF.universal_func(uft, L_MO, SFP.uf_params(param_set))
	_π_group = FT(UF.π_group(uf, transport))
	_π_group⁻¹ = (1 / _π_group)
	R_z0 = 1 - z0(sc, transport) / Δz(sc)
	denom1 = log(Δz(sc) / z0(sc, transport))
	denom2 = -UF.Psi(uf, Δz(sc) / uf.L, transport)
	denom3 = z0(sc, transport) / Δz(sc) * UF.Psi(uf, z0(sc, transport) / uf.L, transport)
	denom4 = R_z0 * (UF.psi(uf, z0(sc, transport) / uf.L, transport) - 1)
	Σterms = denom1 + denom2 + denom3 + denom4
	return _π_group⁻¹ * von_karman_const / Σterms
	end

If there is no specific purpose for Businger, Gryanik then they should be eliminated. Note also that these types are all identical, so even if we want to keep them, there only needs to be one type that holds on to L.

In summary, I propose the following changes:

1. Eliminate the types that are typically notated as uf: Businger, Gryanik, etc
2. Eliminate the "*Type" constructs: BusingerType, etc
3. Use the *Params types for dispatch, where Businger and BusingerType are now used
4. Remove the Params suffix from the existing types to make the lexicon more concise and clear, reflecting the fact that these types both control dispatch and also hold parameter values. Eg BusingerParams should simply be called Businger (or BusingerUniversalFunction to be more verbose).

The first 3 changes are purely internal refactoring, while the last changes the user API.

[glwagner]

If we do undertake such a refactor, it's worth discussing whether we want to clean up the nomenclature at the same time. There's significant confusion generated by widespread conflation of ideas and words: the function surface_conditions takes in an object of type "AbstractSurfaceConditions" and spits out SurfaceFluxConditions. Reusing the words "surface" and "conditions" in this way makes the code quite difficult to understand when reading it. It's important to sharply distinguish between the concept of the "state" (or conditions), and the fluxes that are computed as a function of that state.

There seem to be many other instances of names generating confusion. Another one is the function windspeed, which actually returns the magnitude of the difference between the atmosphere velocity and the surface velocity. The "wind speed", on the other hand, cannot be defined as anything other (in a sane world) than the magnitude of the atmosphere velocity.

Given these pop out just from a casual reading of the code, it would seem that a thorough review is needed to refactor this code, which is critical to our coupled modeling system, into high quality, maintainable, and understandable software...

[glwagner]

@akshaysridhar I'm happy to take this on. If we accept that refactoring is needed then perhaps a place to start is an SDI that proposes a new structure and naming scheme for SurfaceFluxes.jl. I'd also propose splitting the code into more files with more module structure. I find it hard to navigate as is.

[akshaysridhar]

@akshaysridhar I'm happy to take this on. If we accept that refactoring is needed then perhaps a place to start is an SDI that proposes a new structure and naming scheme for SurfaceFluxes.jl. I'd also propose splitting the code into more files with more module structure. I find it hard to navigate as is.

@glwagner Thanks for raising this issue - Agreed - we can start a refactor SDI + propose updated/consistent nomenclature to make integration into other systems more intuitive.