Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update_param! dirties the model definition #753

Closed
davidanthoff opened this issue Sep 9, 2020 · 5 comments · Fixed by #759
Closed

update_param! dirties the model definition #753

davidanthoff opened this issue Sep 9, 2020 · 5 comments · Fixed by #759
Milestone

Comments

@davidanthoff
Copy link
Collaborator

When I do this:

julia> m = MimiFUND.get_model()
Mimi.Model
  Module: Mimi
  Components: 
    ComponentId(MimiFUND.scenariouncertainty)
    ComponentId(MimiFUND.population)
    ComponentId(MimiFUND.geography)
    ComponentId(MimiFUND.socioeconomic)
    ComponentId(MimiFUND.emissions)
    ComponentId(MimiFUND.climateco2cycle)
    ComponentId(MimiFUND.climatech4cycle)
    ComponentId(MimiFUND.climaten2ocycle)
    ComponentId(MimiFUND.climatesf6cycle)
    ComponentId(MimiFUND.climateforcing)
    ComponentId(MimiFUND.climatedynamics)
    ComponentId(MimiFUND.biodiversity)
    ComponentId(MimiFUND.climateregional)
    ComponentId(MimiFUND.ocean)
    ComponentId(MimiFUND.impactagriculture)
    ComponentId(MimiFUND.impactbiodiversity)
    ComponentId(MimiFUND.impactcardiovascularrespiratory)
    ComponentId(MimiFUND.impactcooling)
    ComponentId(MimiFUND.impactdiarrhoea)
    ComponentId(MimiFUND.impactextratropicalstorms)
    ComponentId(MimiFUND.impactforests)
    ComponentId(MimiFUND.impactheating)
    ComponentId(MimiFUND.impactvectorbornediseases)
    ComponentId(MimiFUND.impacttropicalstorms)
    ComponentId(MimiFUND.vslvmorb)
    ComponentId(MimiFUND.impactdeathmorbidity)
    ComponentId(MimiFUND.impactwaterresources)
    ComponentId(MimiFUND.impactsealevelrise)
    ComponentId(MimiFUND.impactaggregation)
  Built: false


julia> m = MimiFUND.get_model()^C

julia> m.md.dirty
true

julia> run(m)

julia> m.md.dirty
false

julia> update_param!(m, :co2pre, 270)
true

julia> m.md.dirty
true

What this means is that code like this:

for i=1:10
  update_param!(m, :foo, rand())
  run(m)
end

will rebuild the model instance in every loop.

That is not how it should be, right? We want to be able to update external parameter values and rerun without rebuilding, it seems to me?

Or do we need to manually construct a model instance in that case and then update the parameter values only in the model instance?

@corakingdon
Copy link
Collaborator

corakingdon commented Sep 9, 2020

The reason it "dirties" the model is because after you update the parameter value, the model definition now does not match the model instance (the model instance still holds computed values from when the parameter in question was equal to the old value). I don't see this as a bug.

However, we have previously discussed adding an unsafe version of update_param! that would allow you to not have to rebuild the model instance, we've just never implemented this (see #476). Thinking about this now though, maybe there should just be a keyword option to update_param! such as: rebuild::Bool = true that a user can override if they know what they are doing? The only danger (and what makes this "unsafe" in my mind) is if a user calls the update function but then doesn't immediately re-run the model.

@corakingdon
Copy link
Collaborator

Alternatively, we could add an option to the run function (something like rebuild_if_dirty::Bool = true) that the user could override instead (if they know that the changes they made that made it dirty are just parameter value changes, and not structural changes like internal connections that need to be re-built)

@davidanthoff
Copy link
Collaborator Author

Or we just start to expose the whole model instance/definition distinction in the public API for advanced use cases? For example, this could be written as:

m  = MimiFUND.get_model()

mi = get_new_instance(m) # I guess this is just build, right?

for i=1:10
  update_param!(mi, :foo, rand())
  run(mi)
end

And so the idea would be that an update_param! on a model instance just updates that parameter value in that specific model instance.

@lrennels lrennels added this to the Triage milestone Sep 9, 2020
@corakingdon
Copy link
Collaborator

@davidanthoff is this resolved now? i.e. does update_param!(mi) work on a model instance still?

@davidanthoff
Copy link
Collaborator Author

No, update_param!(mi) doesn't exist yet.

I think we currently actually don't have the notion that a model instance can use a different instance of values for a parameter than its model definition, right? At least that is how the data structure looks to me right now.

I think what we'll need to do in update_param! is essentially this:

  1. go through the md of the mi to understand which components/parameters pick up that specific external parameter.
  2. for each component that picks this up, replace the value of nt with a new named tuple instance in which the field for the parameter we want to replace points to the value that was passed in via update_param!

I think that should probably do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants