Simulation API modification functions

[lrennels]

Take a second look at modification functions and add documentation/examples as well as wrapper functions for simplicity.

• deleteRV!
• addRV!
• replaceRV!
• deleteTransform!
• addTransform!
• deleteSave!
• addSave!
• set_payload!
• payload

[corakingdon]

Smaller things to resolve:

• I'll plan to rename these function names to our loosely decided upon convention (i.e. add_transform! instead of addTransform!)
• rv names: when using @defsim, the macro uses the function _make_rvname so that a parameter name gets linked to an RV with a name that has an exclamation point and a globally unique number appended to the parameter name. I think this means that in order to use replace_rv!, the user would have to know the number for the RV rather than just the parameter name, which doesn't seem desirable... Should we change replace_rv! to take the paramname instead of the rvname somehow?
• I think currently add_transform! doesn't check for if there is already a transformation for that parameter name. What would happen in this case? and should we catch/error here?
• do we want to support adding RVs for array parameters as well? like we allow for in @defsim? Meaning that we would have an add_rv! function that accepts a vector of distributions (or vector of symbol=>distribution pairs like in @defsim)

Main question about easier user functions:

Which of the following two designs do we want:

1. Add an additional function with a different name like add_rv_and_connect! that calls the existing add_rv! and add_transform! functions (but maybe rename add_rv! to add_rv_only! so it's more clear, or maybe don't export it...)
2. OR do we want to only have one function, add_rv! that has different series of keywords options that indicate whether or not you also want to add the TransformSpec.

My initial though was that I prefer option 2, because I thought it would be simpler and I don't like the names I've come up with for option 1. But option 2 might have more opportunities to be misinterpreted, depending on what the default assumptions are. Here's example code for option 2:

# keep this existing function that takes an already constructed RandomVariable
function add_rv!(sim::SimulationDef, rv::RandomVariable)
    name = rv.name
    haskey(sim.rvdict, name) && error("Simulation def already has RV :$name. Use replaceRV! to replace it.")
    sim.rvdict[name] = rv
    _update_nt_type!(sim)
end

# I would opt for deleting this current method of the function:
# add_rv!(sim::SimulationDef, name::Symbol, dist::Distribution) = add_rv!(sim, RandomVariable(name, dist))

# And then I would add this new method:
function add_rv!(sim::SimulationDef, paramname::Symbol, dist::Distribution, op::Union{Symbol, nothing}=:(=), dims::Vector{T}=[])
    # should we do this to make the rvname have a global number attached?
    rvname = _make_rvname(paramname)

    # add the RV
    add_rv!(sim, RandomVariable(rvname, dist))

    # add TransformSpec if op is provided
    # should we check that it's a valid param name though?
    if op != nothing
        add_transform!(sim, paramname, op, rvname, dims)
    end

end

I think I like this, where the default op is :(=). I think this would be the main use case, but if a user wants to add a new RV without linking it up to any parameter in the model, then they have two options:

• either call this last method and specify op=nothing. In this case, though, the argument paramname wouldn't be associated with a param in the models, but I think that's okay?
• OR they can call the first method with an already constructed RV add_rv!(sim, RV)

@lrennels @rjplevin Thoughts on all of this?

[rjplevin]

I'll plan to rename these function names to our loosely decided upon convention

Agreed. Not sure why I named them in CamelCase (old habits die hard...)

Should we change replace_rv! to take the paramname instead of the rvname somehow?

The problem is that (in the general case), a single array parameter can have different distributions assigned to different slices, and this occurs only for "anonymous" RV used directly. One can always name an RV first, then use that, in which case the name is not modified.

I think currently add_transform! doesn't check for if there is already a transformation for that parameter name. What would happen in this case? and should we catch/error here?

We might error, but provide a kwarg like replace::Bool=false that, if set to true allows overwriting the old transform. Alternatively (or in addition), create replace_transform! to do that.

do we want to support adding RVs for array parameters as well? like we allow for in @defSim? Meaning that we would have an add_rv! function that accepts a vector of distributions (or vector of symbol=>distribution pairs like in @defSim)

I think having a complete, rather than partial, API seems logical. We'd also want to support assigning distributions to array slices. We might consider using macros for this to allow us to support the same syntax used in @defsim

# should we do this to make the rvname have a global number attached?

This is necessary only if assigning to slices or assigning a vector of distributions.

I don't like the default op being = and having to pass op=nothing to avoid it. Given that there's also a method that takes no op arg, I'd rather this method have a positional arg for op, i.e.,

function add_rv!(sim::SimulationDef, paramname::Symbol, dist::Distribution, op::Symbol; dims::Vector{T}=[])

[corakingdon]

Can you explain why you don't like the default op being =? I was just thinking that it will overwhelmingly be the most common use case, whereas if the default is nothing, I'm worried about people making the same mistake I made (thinking they are adding a random variable for a specific parameter name, but then it doesn't get linked to that parameter in the model when they run the simulation)

[rjplevin]

I was just thinking that it will overwhelmingly be the most common use case

In my MCS work, this has not been the most common use case. In sensitivity analysis (as opposed to predictive MCS where the output distribution is assigned probabilistic meaning) I would expect it more common to want to move variables by +/- X% from their default value, rather than having to define distributions that incorporate that default value. So the most common for me is *=.

In any case, I prefer using a macro, using the syntax of @defsim:

# Assign RVs to parameters
@assign_rv!(sim, name *= Normal(x, y))
@assign_rv!(sim, name["USA", 1:4] = Uniform(a, b))

# Create a named RV, but don't assign it to anything
@name_rv!(sim, name1 = Normal(1, 0.2))

# Assign an existing RV to two parameters
@assign_rv!(sim, foo = name1)
@assign_rv!(sim, bar = name1)

and so on. These would just parse the expression and call add_rv! or (new func) create_rv!

[corakingdon]

I've started implementing these macros now. I just pushed a branch sim-update-macros here and the first one starts around line 192: https://github.com/mimiframework/Mimi.jl/blob/sim-update-macros/src/mcs/defmcs.jl (I started off of Lisa's simulation api branch that hasn't been merged into master yet)

I still need to add tests, but I have some questions about intended functionality, particularly relating to replacing existsing RVs and transforms:

• should @assign_rv! work to replace an existing transform spec if one already exists for the specified paramname and dims? or should it error if the name provided already has an assigned RV? I think it should work, because I think it is clear that the user wants to assign a new distribution to the provided paramname, even if one already exists for it. (Currently I've implemented it to delete the old TransformSpec in this case, but does not delete the old RV)
• should @name_rv! work to replace RVs? or should there be a separate @replace_rv! function? (Currently I've implemented it to error in this case...)

[rjplevin]

!($(QuoteNode(rvname)) in keys($sim.rvdict)) &&
error("Random variable "$($(QuoteNode(rvname)))" not found in the provided simulation.")

Simpler:

haskey($sim.rvdict, $(QuoteNode(rvname)) || 
     error("Random variable '$($rvname)' not found in the provided simulation.")

[rjplevin]

should @assign_rv! work to replace an existing transform spec if one already exists for the specified paramname and dims?

should @name_rv! work to replace RVs? or should there be a separate @replace_rv! function?

In both cases, we might want to error if we detect a conflict, unless the user passes replace=true (but if replace == true, do not error if no conflict is found...)

Currently I've implemented it to delete the old TransformSpec in this case, but does not delete the old RV

Agreed about not deleting the RV. Other transforms might still use it, and the user might want to add a new one. We might add a method like Base.delete!(sim, name) to allow cleanup, or just modify the sim loop to drop unbound RVs from the sim copy before running.