Changes to set_param!

[davidanthoff]

The core functions are:

set_param!(m, :compA, :p1, :ext_p2_name, 34.)
update_param!(m, :ext_p2_name, 45.)

And the shortcuts are:

set_param!(m, :p1, 23.)

This function would look at all the components in the model, find all the unbound parameters named p1, create an external parameter called p1, set its value to 23, and create connections from all the external unbound parameters that are named p1 to the external parameter p1. If there is already a p1 in the external parameter list, it errors.

set_param!(m, :compA, :p1, 34.)

This function creates a new external parameter called p1 (if that already exists, it errors), sets its value to 34., and then connects compA/p1 to this external parameter p1.

[davidanthoff]

Maybe also need:

set_param!(m, [:compA, :compC, :compD], :p1, 23)
set_param!(m, [:compA=>:p1, :compC=>:p3], :p_ext_3, 34.)
connect_param!(m, :compA, :p1, :p_ext_3)

[davidanthoff]

set_param! is not a good name for all of this. So we'll come up with new better names for the new functions we creating. We'll leave set_param! around as is with the old behavior, except, that we make

set_param!(m, :compA, :p1, 34.)
set_param!(m, :compB, :p1, 12.)

error.

Rollout is: we add the new fancy functions with better names first, tag a release. Then we migrate models over to that. Then we start to throw errors in a future Mimi release for the buggy set_param! corner case.

[rjplevin]

@davidanthoff, The current design defines external_params in the CompositeComponentDef, so these can exist at all levels, including ModelDef.

This is necessary since a component might have external params and it can't be aware of external params of the same name defined elsewhere in the model. To connect these knowledgeably, I suggest we provide a function that lists all the external params of a given name, ideally with a documentation string, so the modeler can learn whether all the params named :foo serve the same purpose prior to connecting/setting them all.

Should this method apply only to unbound params? What if you want to join together parameters :foo that has the same semantics across multiple components? They might already be bound, but connecting them to a single external param at a higher composite level makes sense.

The existing set_param! methods operate on a single composite comp def. Perhaps the new functions that connect all of the same name should be called set_param_group! or join_params! or something that indicates that this works across component levels? Does it need a flag to indicate whether to operate only on unbound params or on any with the given name?

We will also need a method for disconnecting individual component's parameter from a joined set, or at least to delete the external parameter and all connections to it, forcing the user to recreate a set with fewer parameters.

[corakingdon]

@rjplevin this seems pretty related to the conversation in the second half of this issue as well: #519

(we had some god back and forth about namespaces for parameters in composites and what gets exported, so just wanted to make sure you are thinking about that now too!)

[rjplevin]

@davidanthoff, @ckingdon95, @lrennels: I feel like the overall design of the composites is getting away from me / us. We have a number of discussion threads, but it's hard to distill a set of design decisions from all of this. This is the main stumbling block to finishing the implementation.

I think we may want to do one or both of the following:

1. A code review of the composite version of Mimi. It would be good if we were all up to speed on where we are. The new version necessitated some unforeseen types, like ComponentPath, ParamPath, and ParameterDefReference. (Probably others...)
2. Write up a design document for how composites behave w.r.t. variable and parameter definitions, "percolation" of pars/vars up the hierarchy (including when exactly), start/end behavior, handling of namespace collisions, etc. This will be important developer documentation and it will facilitate the actual development!

[rjplevin]

Some of the guidance above doesn't account for hierarchy, e.g.

set_param!(m, [:compA, :compC, :compD], :p1, 23)
set_param!(m, [:compA=>:p1, :compC=>:p3], :p_ext_3, 34.)

A single symbol is not an adequate descriptor for a component in a hierarchical model. These would have to become either ComponentPath instances (e.g., ComponentPath(:top, :Comp1, :CompA), or the string equivalent ComponentPath("/top/Comp1/CompA"), which is sometimes more convenient. (Though I seem to recall David not caring for the string version...)

Just checking that I've not misunderstood the intention.

[corakingdon]

I definitely agree that the design is getting away from us, and I think we should discuss it in depth this Friday!

While I'm thinking about it now, though, I'm just going to respond to your second comment about hierarchy. This is how I was interpreting our intention: we would no longer use ComponentPath's to set parameters, because all parameters that are not resolved by internal connections automatically get "bubbled up" to the namespace of the parents. So you would only call set_param! on top level components of the model.

I think the way that I wrote the composite version of DICE is consistent with this: https://github.com/anthofflab/MimiDICE2010.jl/pull/24/files

you'll see that all of the calls to set_param in the get_model_composite function look like this:
set_param!(m, :DICE2010, :parname, value)
where :DICE2010 is the "top" component, and the parname came from a leaf component that hadn't been internally connected to anything else.

[corakingdon]

This is exactly what you said in your comment on Jun 17 on that other thread.
#519 (comment)

I repeat, yes this is getting away from us!!

[rjplevin]

Thanks, Cora. I'll have to review all these threads and extract actual decisions so we can review them.

[davidanthoff]

Yes, I think @ckingdon95 has it right: I think we never need to deal with nested external parameter problems. If I use a composite component, then from the outside it should look exactly like a leaf component, i.e. at no point should I be exposed to the fact that internally it is a composite. So that in particular means that I should never be exposed to external parameters that are deeper down in the tree.

Happy to have a meeting dedicated to all of this.

[lrennels]

I’ll look over this today, but agree I’m happy to dedicate a good amount of Friday's meeting to this so we’re all on the same page!

[davidanthoff]

@rjplevin mentioned over in #651 (comment) that

set_param!(m, :example, :p1, 1:5)
set_param!(m, :example, :p2, reshape(1:15, 5, 3))

wouldn't work going forward.

I think the only breaking change to set_param! that we should have is what we discussed above in this issue here. I don't understand why that would mean that a simple set_param! would now no longer work, and I think in fact that it should continue to work.

[rjplevin]

Unsure what a "simple" set_param! is. Two issues here:

set_param!(m, :example, :p1, 1:5) -- I thought we were only going to allow setting params on m, so this method, with sub-component name as 2nd arg would be deprecated.

But as per yesterday's discussion, add_comp! will not trigger imports, since the user may not be done setting up connections yet. So set_param!(m, :p1, 1:5) won't work without one of the alternatives I suggested.

I think the way to go is the "smart" set_param! that imports on demand if the meaning is unambiguous (as I described in #651.)

[rjplevin]

As I wrote in #651,

In the current code under development, the first statement won't work because the import will not have happened yet. We might import on demand if the requested param doesn't exist in m.md and it is unbound in one subcomponent (but not in multiple).

Alternatively, you could call import_params!(m) any time after the add_comp! and before the set_param! calls. Alternatively alternatively, we could add a kwarg, e.g., add_comp!(m, import_all=True).

I think the best approach is the first one above, i.e., the "import on demand".

I thought we decided this form (with component named) was deprecated:

set_param!(m, :example, :p1, 1:5)

Since we're allowing setting params only at the model level.

[davidanthoff]

set_param!(m, :example, :p1, 1:5) -- I thought we were only going to allow setting params on m, so this method, with sub-component name as 2nd arg would be deprecated.

No, the spec we came up with is at the top of this issue here, and it does not deprecate this. I can't find any discussion where we said this would be deprecated (that would be a massively, massively breaking change, so that is just generally not an option). The only thing we said would change is that this situation will throw an error is there is an ambiguity, the details of that are described at the top of this issue.

But as per yesterday's discussion, add_comp! will not trigger imports, since the user may not be done setting up connections yet. So set_param!(m, :p1, 1:5) won't work without one of the alternatives I suggested.

I don't understand this, in particular not that add_comp! would not trigger imports. We only talked about composite comps yesterday, I don't see how any of that discussion is relevant for the behavior of add_comp!, which operates on a model.

At the model level, the only change we want is what is described at the top of this issue, no other breaking changes. I think all of that is entirely independent of the composite stuff, because from the point of view of a model, it really shouldn't matter whether something is a leaf or composite component, from the model composition point of view they should really look the same.

[corakingdon]

I've implemented the core functions described by @davidanthoff in the first comment. Now moving this issue to milestone v1.1 because the methods described in David's second and third comment are not implemented yet, but can be added later as an enhancement.

[jmwingenroth]

The methods in David's 3rd comment and one from the 2nd comment, connect_param!(m, :compA, :p1, :p_ext_3), were implemented, apparently by other commits/PRs not referencing this issue.

PR #800 implements set_param!(m, [:compA, :compC, :compD], :p1, 23). It is on my list to cover set_param!(m, [:compA=>:p1, :compC=>:p3], :p_ext_3, 34.) as well.

[lrennels]

Closing for now since most will be taken care of in new issues, @wingfactor those methods will still be useful we can just incorporate them into the upcoming work!