cgrudz / DataAssimilationBenchmarks.jl

Package Information and Documentation
https://cgrudz.github.io/DataAssimilationBenchmarks.jl/dev/
Apache License 2.0
13 stars 5 forks source link

Favor named argument list over argument tuple #9

Closed peanutfun closed 2 years ago

peanutfun commented 2 years ago

Is there a particular reason why most functions in this package use one tuple as argument instead of multiple arguments? As the tuple effectively causes the arguments to be unnamed, it is very difficult to understand the usage of the functions from just looking at their signature. Since the tuple arguments are usually unpacked right away (see, e.g., /src/experiments/GenerateTimeSeries.jl#L18), I see no reason why the function argument list should not be used to define these variables in the first place.

Example:

# Now:
function L96_time_series(args::Tuple{Int64,Int64,Float64,Int64,Int64,Float64,Float64})
    seed, state_dim, tanl, nanl, spin, diffusion, F = args
    # Do stuff...
end

# Better, imo:
function L96_time_series(
    seed::Int64,
    state_dim::Int64,
    tanl::Float64,
    nanl::Int64,
    spin::Int64,
    diffusion::Float64,
    F::Float64
)
    # Do stuff...
end

openjournals/joss-reviews#4129

cgrudz commented 2 years ago

This was a very important point, and many thanks for bringing this up. The code base has now been restructured in favor of named tuples. In particular, this solves the issue of nameless arguments, but also allows one to use the parameter map constructors as defined in ParallelExperimentDriver: https://cgrudz.github.io/DataAssimilationBenchmarks.jl/dev/submodules/experiments/ParallelExperimentDriver/ This allows one to abstract hyper-parameter grids into named tuples with a simple loop constructor and apply the native Distributed parallel mapping to run a large grid of experiment configurations in naive parallelism.