EconForge / Dolo.jl

Economic modeling in Julia
Other
57 stars 29 forks source link

FIX: set_calibration was not working. #97

Closed albop closed 7 years ago

albop commented 7 years ago

@sglyon, @azheng25 : this fixes two things:

Not that we still change one parameter at a time.

sglyon commented 7 years ago

Looks good. Tests would be best, but if you don't have time now we can come back around later

albop commented 7 years ago

You guessed right...

On Mon, 17 Jul 2017, 17:48 Spencer Lyon, notifications@github.com wrote:

Looks good. Tests would be best, but if you don't have time now we can come back around later

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/EconForge/Dolo.jl/pull/97#issuecomment-315808468, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ5Kc9Le18ZWnFkf0MNe74RaBCO-sfhks5sO48rgaJpZM4OaLov .

albop commented 7 years ago

@sglyon All right, revisiting, with a bit more good will I made two additions:

sglyon commented 7 years ago

Nice!! Thank you for adding tests.

I also like the keyword argument version. The API seems very flexible now.

sglyon commented 7 years ago

What was your question about the type of the Dict? I didn't totally follow

albop commented 7 years ago

The versions with dictionary arguments are: function set_calibration!(model::Model, values) and

function set_calibration!(model::Model; kwargs...)
    set_calibration!(model, kwargs)
end

Initially I wrote

function set_calibration!(model::Model, values::Dict{Symbol,Union{Real,Expr, Symbol}})

but it didn't work.

sglyon commented 7 years ago

Ahh I see. I think the issue is that splatted keyword arguments are not thrown into a Dict by default. I think this method definition should work:

function set_calibration!(model::Model; kwargs...)
    set_calibration!(model, Dict(kwargs))
end

function set_calibration!{T}(model::Model, values::Associative{Symbol,T})
    for (key,value) in values
        model.data[:calibration][key] = value
    end
    model.calibration = get_calibration(model)
    model.exogenous = get_exogenous(model)
    model.domain = get_domain(model)
    # model.options = get_options(model) # we don't substitute calib here
    model.grid = get_grid(model)
    model.calibration
end

but I haven't tested that out so I'm only like 98% sure

albop commented 7 years ago

This works, so I made the change.

sglyon commented 7 years ago

Will merge when green