change replace_comp! to replace!

[corakingdon]

issue #712

I changed the functionality from:

replace_comp!(m, new, old)

to:

replace!(m, old => new)

as discussed in order to follow Julia conventions for Base.replace!

One question for now:
There are three methods of replace! in Mimi:

replace!(m::Model, old_new::Pair{Symbol, ComponentId}; kwargs...)
replace!(m::Model, old_new::Pair{Symbol, ComponentDef}; kwargs...)
replace!(obj::AbstractCompositeComponentDef, old_new::Pair{Symbol, ComponentId}; kwargs...)    # `obj` can be a ModelDef or a CompositeComponentDef

Users can call either of the first two methods on a model, using either the ComponentId or the ComponentDef of the new component, and then Mimi calls the third method on the model definition, using the ComponentId. The third method also supports the ability to replace a subcomponent within a composite component definition if called directly, although we don't have any use cases of this yet. My question is whether we should add an additional method, so that someone can use the ComponentDef instead of the ComponentId in this case if desired:

replace!(obj::AbstractCompositeComponentDef, old_new::Pair{Symbol, ComponentDef}; kwargs...) 

Replacing composite components?
I believe you cannot currently use replace! to replace a composite component (it only works on ComponentDefs, not AbstractComponentDefs). I'm not sure how trivial this would be to implement, since we might need to add additional checks about behavior of subcomponents, or it might just work out nicely with our new parameter/variable definitions in composites. Maybe this can be added in a later version, although there are not currently helpful error messages if someone tried to use this. I'd just like input on whether I should spend more time thinking about this now or not.

Another question that can probably be for a later version:
Do we want to add an optional keyword argument for changing the name of the added component in the model's list of component names? It currently gets added under the replaced component's name. This would be slightly non-trivial change, because I think you would have to also change any instances of the name that's stored in parameter connections etc.

[codecov]

Codecov Report

Merging #715 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
+ Coverage   79.73%   79.81%   +0.07%     
==========================================
  Files          38       38              
  Lines        2877     2888      +11     
==========================================
+ Hits         2294     2305      +11     
  Misses        583      583              

Flag	Coverage Δ
#unittests	79.81% <100.00%> (+0.07%)	⬆️

Impacted Files	Coverage Δ
src/core/defs.jl	83.08% <100.00%> (+0.05%)	⬆️
src/core/model.jl	86.04% <100.00%> (+4.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f399f7...460a34b. Read the comment docs.

[davidanthoff]

So this is a bit embarassing, but I don't know what the difference between ComponentId and ComponentDef is :) My expectation would be that one can only use the same identifier that one uses in addcomp! as the second thing in the Pair, or is that wrong?

I think we should just flat out disable replacement within composite components for now. We can revisit in later version, but for 1.0 I think we really don't need it.

I think for a first version we can also not have the option to rename things. Actually, maybe we don't need that at all, I would say :)

I'm wondering whether the third method in your list should actually be a distinct function that is more of an internal function, and not part of the public API?

[corakingdon]

@davidanthoff The @defcomp macro creates a variable of with the component's name and assigns the ComponentDef to it. So the ComponentDef is what's used during add_comp!. A ComponentId stores the module in which the component was defined, as well as the name of the component.

I think I agree with your proposed solution: only allow using a ComponentDef for replace!, and rename the internal method (the third method listed above) so that it can't be called by users on CompositeComponentDefs.

[davidanthoff]

Great!