alan-turing-institute / PDSampler.jl

Piecewise Deterministic Sampler library (Bouncy particle sampler, Zig Zag sampler, ...)
Other
33 stars 8 forks source link

make all arguments of Simulation and LocalSimulation explicit #12

Closed tlienart closed 7 years ago

tlienart commented 7 years ago

At the moment some of the arguments are named and some are not. I feel a user is bound to forget the order of the arguments and would end up having to copy paste etc (this may happen anyway).

It may be a good idea to refactor into something like

Simulation(dict)

where the dict looks then like

simparams = Dict("x0"=>x0, "v0"=>v0)

etc.

The question is then whether it makes sense to have a full immutable type for the simulation and whether it would not be better to just have a dictionary.

martintoreilly commented 7 years ago

👍 on wanting to support specifying all parameters by name. Both R and Python allow passing required parameters as named arguments so it was surprising to learn Julia doesn't.

tlienart commented 7 years ago

Julia definitely supports named arguments, eg.:

foo(;a=5,b=7) = a+b
foo(a=3)

so you just separate named and unnamed arguments via the semicolon. All named arguments require a default value. This is why I'm thinking a dictionary may be better with a bunch of assertions to check that all entries are present and valid (indeed it's unclear what a good default value is in multiple cases)

tlienart commented 7 years ago

This is now done in LocalSimulation (9d030ab) by overloading the constructor allowing for named arguments everywhere. It required defining "emptytypes" for FactorGraph etc...

function LocalSimulation(;
            factorgraph = emptyfactorgraph(),
            x0          = zeros(0),
            v0          = zeros(0),
            T           = 0.0,
            maxnevents  = 0,
            lambdaref   = 0)
    LocalSimulation(factorgraph, x0, v0, T, maxnevents, lambdaref)
end

And this can be called like so:

ls2 = LocalSimulation(
        factorgraph=chain,
        x0=x0, v0=v0, T=T,
        maxnevents=1000, lambdaref=0.001)

This is backward compatible. Will do the same with Simulation.

tlienart commented 7 years ago

Exactly the same approach for simulation. The minimal call should be well documented but that's inevitable.