add deep kwarg to delete!

[corakingdon]

#752

Two additions in this PR:

1. This adds delete_unbound_comp_params::Bool=false keyword argument to the delete! function, which when set to true then deletes all external parameters that were only connected to the deleted component.
2. I also created a new function delete_param!(m, external_param_name, delete_connections=true) for removing parameters from a model's list of external parameters.

To discuss:

• kwarg name: I went with the verbose delete_unbound_comp_params for now, which is definitely not perfect. The reason I'm feeling averse to deep or recursive is that it doesn't delete all parameters associated with this component, only the ones not connected to any other components.
• in order to add this functionality, I had to change the function signature to only operate on ModelDefs, and not on all AbstractCompositeComponentDefs because the latter do not have external parameter lists.
• for the new delete_param! function, is this okay to make the default delete_connections=true, and do we want a different name for that keyword arg?

[codecov]

Codecov Report

Merging #774 into master will increase coverage by 0.05%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master     #774      +/-   ##
==========================================
+ Coverage   78.68%   78.73%   +0.05%     
==========================================
  Files          35       35              
  Lines        2984     2996      +12     
==========================================
+ Hits         2348     2359      +11     
- Misses        636      637       +1     

Flag	Coverage Δ
#unittests	78.73% <94.44%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Mimi.jl	100.00% <ø> (ø)
src/core/model.jl	82.85% <ø> (ø)
src/core/defs.jl	82.91% <94.44%> (+0.30%)	⬆️

---

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 555b493...b55d09f. Read the comment docs.

[davidanthoff]

For the new delete_param! function, I don't fully understand what delete_connections=true does. Don't we always have to delete connections that included the deleted parameter, because otherwise we would have "stray" connections hanging around?

[corakingdon]

@davidanthoff Yes, it would be a weird behavior to leave the connections hanging. Shall I remove the option all together for now? (Since this is a new function, we could always add in options later if we realize they might be needed)

[corakingdon]

@davidanthoff ok I got impatient and was assuming you were going to answer yes so I removed that option for now

[corakingdon]

One design question is whether it would be better to not have a separate delete_param! function and if this functionality should be folded into the delete! function. basically it would be:

function Base.delete!(md::ModelDef, item_name::Symbol)
    if item isa component
        _delete_component . . .
    elseif item isa param
        _delete_param . . .
    end
end

The only thing is that we now have this delete_unbound_comp_params option that only makes sense in the component case

[corakingdon]

decided in group meeting: stick with delete_param! because we don't have a shared namespace for components and parameters