raddleverse / MimiCIAM.jl

MIT License
3 stars 1 forks source link

get_model() function #14

Closed lrennels closed 4 years ago

lrennels commented 4 years ago

Hi @cledna, this looks great and almost ready to be put into our Mimi registry. I see that you have a get_model() function with the signature:


function get_model(initparams,xsc,params,t::Int=20)
    run_name = initparams["run_name"]

    m=Model()

    set_dimension!(m, :time, t)
    set_dimension!(m, :adaptPers, length(params["at"]))  
    set_dimension!(m, :regions, xsc[2])
    set_dimension!(m, :segments, xsc[3])

    buildciam(m)
    initciam(xsc, params, m, t)

    return m 

end

and was wondering if it would be possible to slightly amend this so that the signature is

function get_model(;initparams=default_init_params,xsc=default_xsc,params= default_params,t::Int=20)

or something so that all parameters have a default and it's thus possible to call m = get_model()?

cledna commented 4 years ago

Hi Lisa, I made a modification in the latest PR. The function now requires no input parameters, but outputs a tuple of the model, the segment-region dictionary (xsc), and the initialization parameters (initparams). I'm using that structure because the xsc and initparams are needed for the write_ciam function, but if you have insights on how to better organize that I'm happy to brainstorm.

lrennels commented 4 years ago

Great! I talked a bit with the team and I think we would prefer if the get_model() function returned only a model, so that we can call something like:

m = get_model()
run(m)

I think we could do a few things to return those other two outputs, including one or two functions like get_xsc() and get_initparams() or something ... or just one that returns a two-tuple? Just so I understand a little bit better, what is write_ciam? Is that something users will frequently want to call?

tonyewong commented 4 years ago

The run_model() function requires the model object and the xsc object, so there probably isn't any actual need to have that as an additional output. That said, it isn't the worst idea to have xsc as an extra output as part of a tuple in order to bundle up model and inputs. I'm happy either way, but if having only the m as the output is more consistent with other Mimi components, that might be cleanest.

write_ciam should be a void function that writes a csv output file, called after running the model. So I'd say it will be used fairly frequently.

cledna commented 4 years ago

Thanks for the comments! I wonder if it's possible to store initparams and xsc outputs as part of the model structure, separate from the main model? The issue is that both are used in the write_ciam() function to write results to a CSV file. Initparams contains information about how the model was initialized (e.g., the RCP used, the name of the run, whether the full model or a subset was run) which is important to preserve for the user, and xsc contains the segment-region mapping and the names of each region and segment. The full xsc output is not stored in CIAM because the Mimi structure cannot hold strings (I'm not sure if this is still true, though), and it's necessary to translate the segment and region numeric identifiers to their full names in the CSV file. I think I can probably figure out a workaround for this, but it would be easier to just store them somehow with the model when it is initialized.

lrennels commented 4 years ago

Interesting ok, so these are essentially input metadata that are specific to the model and you will want to access for other functions like write_ciam(). Maybe in this case it would be good to have a function get_model() that only returns a Model, and then a different function like get_inputs that takes the same parameters that returns a tuple of inputs that could then get passed to write_ciam(). So your functions would be something like:

function get_model(;initparams=default_init_params,xsc=default_xsc,params=default_params,t::Int=20)

function get_inputs(;initparams=default_init_params,xsc=default_xsc,params=default_params,t::Int=20)

Then if you had the case where you use defaults, you would have something like:

m1 = get_model()
m1_inputs = get_inputs()
run(m1)
write_ciam(m1, m1_inputs)

and if you wanted non defaults you could use the keyword arguments? That's one idea, I'm sure we can come up with a nice elegant ones that fits everyone's needs!

@davidanthoff @rjplevin @ckingdon you might want to weigh in

cledna commented 4 years ago

I made a workaround that doesn't require additional inputs; it is now in the latest PR.