From 0105c5b18d9d25f326c9ae86b9317044f87c39bf Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 24 Mar 2020 15:59:23 -0400
Subject: [PATCH 01/25] First pass at changes on the definitions side

---
 src/core/connections.jl  |  62 +++++++++++++++-----
 src/core/defcomposite.jl | 110 +++++++++++++++++++++--------------
 src/core/defs.jl         | 120 +++++++++++++++++++++------------------
 src/core/paths.jl        |   5 --
 src/core/types/defs.jl   |  82 ++++++++++++++------------
 5 files changed, 227 insertions(+), 152 deletions(-)

diff --git a/src/core/connections.jl b/src/core/connections.jl
index bef4bc52f..546b9478e 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -54,7 +54,7 @@ end
 verify_units(unit1::AbstractString, unit2::AbstractString) = (unit1 == unit2)
 
 function _check_labels(obj::AbstractCompositeComponentDef,
-                       comp_def::ComponentDef, param_name::Symbol, ext_param::ArrayModelParameter)
+                       comp_def::AbstractComponentDef, param_name::Symbol, ext_param::ArrayModelParameter)
     param_def = parameter(comp_def, param_name)
 
     t1 = eltype(ext_param.values)
@@ -111,7 +111,7 @@ function connect_param!(obj::AbstractCompositeComponentDef, comp_name::Symbol,
     connect_param!(obj, comp_def, param_name, ext_param_name, check_labels=check_labels)
 end
 
-function connect_param!(obj::AbstractCompositeComponentDef, comp_def::ComponentDef,
+function connect_param!(obj::AbstractCompositeComponentDef, comp_def::AbstractComponentDef,
                         param_name::Symbol, ext_param_name::Symbol; check_labels::Bool=true)
     ext_param = external_param(obj, ext_param_name)
 
@@ -334,38 +334,72 @@ function dereferenced_conn(obj::AbstractCompositeComponentDef,
     return ipc
 end
 
+# """
+#     connection_refs(obj::AbstractCompositeComponentDef)
+
+# Return a vector of ParameterDefReferences to parameters with connections.
+# The references are always to the original Parameter definition in a leaf
+# component.
+# """
+# function connection_refs(obj::AbstractCompositeComponentDef)
+#     refs = ParameterDefReference[]
+#     root = get_root(obj)
+#     # @info "root of $(obj.comp_id) is $(root.comp_id))"
+
+#     function _add_conns(obj::AbstractCompositeComponentDef)
+#         append!(refs, [param_ref(root, conn) for conn in obj.internal_param_conns])
+#     end
+
+#     recurse(obj, _add_conns; composite_only=true)
+
+#     if obj isa ModelDef
+#         append!(refs, [param_ref(obj, epc) for epc in external_param_conns(obj)])
+#     end
+#     return refs
+# end
+
 """
     connection_refs(obj::AbstractCompositeComponentDef)
 
-Return a vector of ParameterDefReferences to parameters with connections.
-The references are always to the original Parameter definition in a leaf
-component.
+Return a vector of UnnamedReference's to parameters with connections.
 """
 function connection_refs(obj::AbstractCompositeComponentDef)
-    refs = ParameterDefReference[]
-    root = get_root(obj)
-    # @info "root of $(obj.comp_id) is $(root.comp_id))"
+    refs = UnnamedReference[]
 
-    function _add_conns(obj::AbstractCompositeComponentDef)
-        append!(refs, [param_ref(root, conn) for conn in obj.internal_param_conns])
+    for item in values(obj.namespace)
+        if item isa CompositeParameterDef
+            for ref in item.refs
+                push!(refs, ref)
+            end
+        end
     end
 
-    recurse(obj, _add_conns; composite_only=true)
-
     if obj isa ModelDef
         append!(refs, [param_ref(obj, epc) for epc in external_param_conns(obj)])
+        error("CK need to implement")
     end
+
     return refs
 end
 
+# """
+#     unconnected_params(obj::AbstractCompositeComponentDef)
+
+# Return a list of ParameterDefReferences to parameters that have not been connected
+# to a value.
+# """
+# function unconnected_params(obj::AbstractCompositeComponentDef)
+#     return setdiff(leaf_params(obj), connection_refs(obj))
+# end
+
 """
     unconnected_params(obj::AbstractCompositeComponentDef)
 
-Return a list of ParameterDefReferences to parameters that have not been connected
+Return a list of UnnamedReference's to parameters that have not been connected
 to a value.
 """
 function unconnected_params(obj::AbstractCompositeComponentDef)
-    return setdiff(leaf_params(obj), connection_refs(obj))
+    return setdiff(subcomp_params(obj), connection_refs(obj))
 end
 
 """
diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index 5a6b3c4fb..217be4924 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -35,8 +35,6 @@ using MacroTools
 # 3. pname_ = Parameter(args__) # args can be: pname, comp.pname, or keyword=value
 # 4. connect(a.param, b.var)
 #
-#
-# @defcomposite should just emit all the same API calls one could make manually
 #
 
  # splitarg produces a tuple for each arg of the form (arg_name, arg_type, slurp, default)
@@ -80,7 +78,8 @@ end
 # Convert @defcomposite "shorthand" statements into Mimi API calls
 #
 function _parse(expr)
-    valid_keys = (:default, :description, :visability, :unit)
+    # valid_keys = (:default, :description, :visability, :unit)
+    valid_keys = (:default, :description, :unit)
     result = nothing
 
     if @capture(expr, newname_ = Component(compname_)) ||
@@ -206,14 +205,10 @@ function import_params!(obj::AbstractCompositeComponentDef;
                         names::Union{Nothing,Vector{Symbol}}=nothing)
 
     unconn = unconnected_params(obj)
-    params = parameters(obj)
-
-    # remove imported params from list of unconnected params
-    unconn = setdiff(unconn, params)
-    # filter!(param_ref -> !(param_ref in params), unconn)
 
     # verify that all explicit names are importable
     if names !== nothing
+        error("CK: need to implement/verify what this behavior is")
         unconn_names = [nameof(param_ref) for param_ref in unconn]
         unknown = setdiff(names, unconn_names)
         if ! isempty(unknown)
@@ -221,25 +216,69 @@ function import_params!(obj::AbstractCompositeComponentDef;
         end
     end
 
+    unique_names = Set([ref.datum_name for ref in unconn])
+    length(unique_names) == length(unconn) || error("There are unresolved parameter name collisions from subcomponents.")
+
     for param_ref in unconn
-        # @info "importing $param_ref to $(obj.comp_id)"
-        name = nameof(param_ref)
-        if names === nothing || name in names
-            obj[name] = param_ref
+        name = param_ref.datum_name
+        haskey(obj, name) && error("Failed to auto-import parameter :$name from component :$(param_ref.comp_name), this name has already been defined in the Composite component's namesapce.")
+        obj[name] = CompositeParameterDef(obj, param_ref)
+    end
+end
+
+# `kwargs` contains the keywords specified by the user when defining the composite parameter in @defcomposite.
+# If the user does not provide a value for one or any of the possible fields, this function looks at the fields 
+# of the subcomponents' parameters to use, but errors if any of them are in conflict.
+# Note that :dim_names and :datatype can't be specified at the composite level, but must match from the subcomponents.
+function _resolve_composite_parameter_kwargs(obj::AbstractCompositeComponentDef, kwargs::Dict{Symbol, Any}, pairs::Vector{Pair{T, Symbol}}, parname::Symbol)  where T <: AbstractComponentDef
+    
+    composite_parameter_conflict_fields = (:default, :description, :unit, :dim_names, :datatype)
+
+    # Access the subcomponents' ParameterDef's specified in `pairs` to be used below for field-checking
+    pardefs = [comp.namespace[param_name] for (comp, param_name) in pairs]
+
+    # Create a new dictionary of resolved values to return
+    new_kwargs = Dict{Symbol, Any}()
+
+    for f in composite_parameter_conflict_fields
+        try 
+            new_kwargs[f] = kwargs[f] # Get the user specified value for this field if there is one
+        catch e
+            # If the composite definition does not specify a value, then need to look to subcomponents and resolve or error
+            subcomponent_set = Set([getproperty(pardef, f) for pardef in pardefs]) # could add an optional "if" here if we want to exclude nothing's or ""'s
+            n = length(subcomponent_set)
+            if n == 1
+                new_kwargs[f] = collect(subcomponent_set)[1]
+            elseif n == 0
+                new_kwargs[f] = nothing
+            else
+                error("Cannot build composite parameter :$parname, subcomponents have conflicting values for the \"$f\" field.")
+            end
         end
     end
+
+    return new_kwargs
 end
 
-# Return the local name of an already-imported parameter, or nothing if not found
-function _find_param_ref(obj, dr)
-    for (name, param_ref) in param_dict(obj)
-        # @info "Comparing refs $param_ref == $dr"
-        if param_ref == dr
-            # @info "Found prior import to $dr named $name"
-            return name
+function _is_connected(obj::AbstractCompositeComponentDef, comp_name::Symbol, datum_name::Symbol)
+    for (k, item) in obj.namespace
+        if isa(item, AbstractCompositeParameterDef)
+            for ref in item.refs
+                if ref.comp_name == comp_name && ref.datum_name == datum_name
+                    return true
+                end
+            end
+        elseif isa(item, AbstractCompositeVariableDef)
+            ref = item.ref
+            if ref.comp_name == comp_name && ref.datum_name == datum_name
+                return true
+            end            
         end
     end
-    nothing
+    return false
+
+    # cannot use the following, because all parameters haven't bubbled up yet
+    # return UnnamedReference(comp_name, datum_name) in unconnected_params(obj)
 end
 
 function import_param!(obj::AbstractCompositeComponentDef, localname::Symbol,
@@ -247,39 +286,24 @@ function import_param!(obj::AbstractCompositeComponentDef, localname::Symbol,
 
     print_pairs = [(comp.comp_id, name) for (comp, name) in pairs]
     # @info "import_param!($(obj.comp_id), :$localname, $print_pairs)"
+    # @info "kwargs: $kwargs"
 
     for (comp, pname) in pairs
 
         if comp == :*       # wild card
-            @info "Got wildcard for param $pname (Not yet implemented)"
+            error("Got wildcard component specification (*) for param $pname (Not yet implemented)")
         else
             compname = nameof(comp)
             has_comp(obj, compname) ||
                 error("_import_param!: $(obj.comp_id) has no element named $compname")
 
-            newcomp = obj[compname]
-
-            dr = datum_reference(newcomp, pname)
-            old_name = _find_param_ref(obj, dr)
-
-            # TBD: :allow_overwrite is not yet passed from @defcomposite
-            key = :allow_overwrite
-            if old_name === nothing || (haskey(kwargs, key) && kwargs[key])
-                # import the parameter from the given component
-                obj[localname] = dr = datum_reference(newcomp, pname)
-                # @info "import_param! created dr $dr"
-            else
-                error("Duplicate import of $dr as $localname, already imported as $old_name. ",
-                      "To allow duplicates, use Parameter($(nameof(comp)).$pname; :$key=True)")
-            end
-
-            if haskey(kwargs, :default)
-                root =  get_root(obj)
-                ref = ParameterDefReference(pname, root, pathof(newcomp), kwargs[:default])
-                save_default!(obj, ref)
-            end
+            _is_connected(obj, compname, pname) &&
+                error("Duplicate import of $(comp.name).$pname")
         end
     end
+    new_kwargs = _resolve_composite_parameter_kwargs(obj, Dict{Symbol, Any}(kwargs), collect(pairs), localname)
+
+    obj[localname] = CompositeParameterDef(localname, pathof(obj), collect(pairs), new_kwargs)
 end
 
 """
@@ -292,7 +316,7 @@ function _import_var!(obj::AbstractCompositeComponentDef, localname::Symbol,
     end
 
     comp = @or(find_comp(obj, path), error("$path not found from component $(obj.comp_id)"))
-    obj[localname] = datum_reference(comp, vname)
+    obj[localname] = CompositeVariableDef(localname, pathof(obj), comp, vname)
 end
 
 nothing
diff --git a/src/core/defs.jl b/src/core/defs.jl
index e6b61c963..d711d3157 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -175,26 +175,26 @@ function _save_to_namespace(comp::AbstractComponentDef, key::Symbol, value::Name
     comp.namespace[key] = value
 end
 
-"""
-    datum_reference(comp::ComponentDef, datum_name::Symbol)
+# """
+#     datum_reference(comp::ComponentDef, datum_name::Symbol)
 
-Create a reference to the given datum, which must already exist.
-"""
-function datum_reference(comp::ComponentDef, datum_name::Symbol)
-    obj = _ns_get(comp, datum_name, AbstractDatumDef)
-    path = @or(obj.comp_path, ComponentPath(comp.name))
-    ref_type = obj isa ParameterDef ? ParameterDefReference : VariableDefReference
-    return ref_type(datum_name, get_root(comp), path)
-end
+# Create a reference to the given datum, which must already exist.
+# """
+# function datum_reference(comp::ComponentDef, datum_name::Symbol)
+#     obj = _ns_get(comp, datum_name, AbstractDatumDef)
+#     path = @or(obj.comp_path, ComponentPath(comp.name))
+#     ref_type = obj isa ParameterDef ? ParameterDefReference : VariableDefReference
+#     return ref_type(datum_name, get_root(comp), path)
+# end
 
-"""
-    datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
+# """
+#     datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
 
-Create a reference to the given datum, which itself must be a DatumReference.
-"""
-function datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
-    _ns_get(comp, datum_name, AbstractDatumReference)
-end
+# Create a reference to the given datum, which itself must be a DatumReference.
+# """
+# function datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
+#     _ns_get(comp, datum_name, AbstractDatumReference)
+# end
 
 function Base.setindex!(comp::AbstractCompositeComponentDef, value::CompositeNamespaceElement, key::Symbol)
     _save_to_namespace(comp, key, value)
@@ -346,20 +346,24 @@ parameter_names(md::ModelDef, comp_name::Symbol) = parameter_names(compdef(md, c
 
 parameter_names(comp_def::AbstractComponentDef) = collect(keys(param_dict(comp_def)))
 
-parameter(obj::ComponentDef, name::Symbol) = _ns_get(obj, name, ParameterDef)
+# parameter(obj::ComponentDef, name::Symbol) = _ns_get(obj, name, ParameterDef)
+
+# parameter(obj::AbstractCompositeComponentDef,
+#           name::Symbol) = dereference(_ns_get(obj, name, ParameterDefReference))
 
-parameter(obj::AbstractCompositeComponentDef,
-          name::Symbol) = dereference(_ns_get(obj, name, ParameterDefReference))
+parameter(obj::AbstractComponentDef, name::Symbol) = _ns_get(obj, name, AbstractParameterDef)
 
 parameter(obj::AbstractCompositeComponentDef, comp_name::Symbol,
           param_name::Symbol) = parameter(compdef(obj, comp_name), param_name)
 
 parameter(dr::ParameterDefReference) = parameter(compdef(dr), nameof(dr))
 
-has_parameter(comp_def::ComponentDef, name::Symbol) = _ns_has(comp_def, name, ParameterDef)
+# has_parameter(comp_def::ComponentDef, name::Symbol) = _ns_has(comp_def, name, ParameterDef)
+
+# has_parameter(comp_def::AbstractCompositeComponentDef,
+#               name::Symbol) = _ns_has(comp_def, name, ParameterDefReference)
 
-has_parameter(comp_def::AbstractCompositeComponentDef,
-              name::Symbol) = _ns_has(comp_def, name, ParameterDefReference)
+has_parameter(comp_def::AbstractComponentDef, name::Symbol) = _ns_has(comp_def, name, AbstractParameterDef)
 
 function parameter_unit(obj::AbstractComponentDef, param_name::Symbol)
     param = parameter(obj, param_name)
@@ -496,12 +500,27 @@ function recurse(obj::ComponentDef, f::Function, args...;
     nothing
 end
 
-function leaf_params(obj::AbstractCompositeComponentDef)
-    params = []
-    recurse(obj, x->append!(params, parameters(x)); leaf_only=true)
+# function leaf_params(obj::AbstractCompositeComponentDef)
+#     params = []
+#     recurse(obj, x->append!(params, parameters(x)); leaf_only=true)
 
-    root = get_root(obj)
-    return [ParameterDefReference(nameof(param), root, pathof(param)) for param in params]
+#     root = get_root(obj)
+#     return [ParameterDefReference(nameof(param), root, pathof(param)) for param in params]
+# end
+
+# return UnnamedReference's for all subcomponents' parameters
+function subcomp_params(obj::AbstractCompositeComponentDef)
+    params = UnnamedReference[]
+    for (name, sub_obj) in obj.namespace
+        if sub_obj isa ComponentDef
+            for (subname, curr_obj) in sub_obj.namespace
+                if curr_obj isa AbstractParameterDef
+                    push!(params, UnnamedReference(name, subname))
+                end
+            end
+        end
+    end
+    return params
 end
 
 """
@@ -585,17 +604,12 @@ model's index labels.
 """
 function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
     # search immediate subcomponents for this parameter
-    found = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
+    found_comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
 
     if ! has_parameter(md, param_name)
-        count = length(found)
+        count = length(found_comps)
         if count >= 1
-            comp = found[1]
-            # @info "Found one child with $param_name to auto-import: $(comp.comp_id)"
-            import_param!(md, param_name, comp => param_name)
-
-        # elseif count > 1
-        #     # error("Can't set parameter :$param_name; found in multiple components")
+            import_param!(md, param_name, [comp => param_name for comp in found_comps]...)
         else # count == 0
             error("Can't set parameter :$param_name; not found in ModelDef or children")
         end
@@ -609,13 +623,13 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
         check_parameter_dimensions(md, value, dims, param_name)
     end
 
-    param_ref = md[param_name]
-    comp_def = compdef(param_ref)
-    param = dereference(param_ref)
-    param_dims = dim_names(param)
+    param_def = md[param_name]
+    comp_name = found_comps[1].name
+    comp_def = md[comp_name]
+    param_dims = param_def.dim_names
     num_dims = length(param_dims)
 
-    data_type = param.datatype
+    data_type = param_def.datatype
     dtype = Union{Missing, (data_type == Number ? number_type(md) : data_type)}
 
     if num_dims > 0
@@ -628,7 +642,7 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
             value_dims = length(size(value))
             if num_dims != value_dims
                 error("Mismatched data size for a set parameter call: dimension :$param_name",
-                      " in $(pathof(comp_def)) has $num_dims dimensions; indicated value",
+                      " in $comp_name has $num_dims dimensions; indicated value",
                       " has $value_dims dimensions.")
             end
             value = convert(Array{dtype, num_dims}, value)
@@ -668,12 +682,8 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
     end
 
     # connect_param! calls dirty! so we don't have to
-    if length(found) == 1
-        connect_param!(md, comp_def, nameof(param_ref), param_name)
-    else
-        for comp_def in found
-            connect_param!(md, comp_def, param_name, param_name)
-        end
+    for comp in found_comps
+        connect_param!(md, comp, param_name, param_name)
     end
     nothing
 end
@@ -984,13 +994,13 @@ function add_comp!(obj::AbstractCompositeComponentDef,
     _insert_comp!(obj, comp_def, before=before, after=after)
 
     # Set parameters to any specified defaults, but only for leaf components
-    if is_leaf(comp_def)
-        for param in parameters(comp_def)
-            if param.default !== nothing
-                save_default!(obj, comp_def, param)
-            end
-        end
-    end
+    # if is_leaf(comp_def)
+    #     for param in parameters(comp_def)
+    #         if param.default !== nothing
+    #             save_default!(obj, comp_def, param)
+    #         end
+    #     end
+    # end
 
     # Return the comp since it's a copy of what was passed in
     return comp_def
diff --git a/src/core/paths.jl b/src/core/paths.jl
index ffecac76b..0e14797cb 100644
--- a/src/core/paths.jl
+++ b/src/core/paths.jl
@@ -70,11 +70,6 @@ function _fix_comp_path!(child::AbstractComponentDef, parent::AbstractCompositeC
             end
         end
 
-        defaults = child.defaults
-        for (i, ref) in enumerate(defaults)
-            ref_comp = find_comp(parent, pathof(ref))
-            defaults[i] = ParameterDefReference(ref.name, root, pathof(ref_comp), ref.default)
-        end
 
     else
         for datum in [variables(child)..., parameters(child)...]
diff --git a/src/core/types/defs.jl b/src/core/types/defs.jl
index 449c8db0e..605ee8655 100644
--- a/src/core/types/defs.jl
+++ b/src/core/types/defs.jl
@@ -103,6 +103,51 @@ struct SubComponent <: MimiStruct
     alias::Union{Nothing, Symbol}
 end
 
+struct UnnamedReference
+    # root::AbstractComponentDef
+    # comp_path::ComponentPath
+    comp_name::Symbol
+    datum_name::Symbol
+end
+
+@class CompositeParameterDef <: ParameterDef begin
+    refs::Vector{UnnamedReference}
+end
+
+# Create a CompositeParameterDef from a list of compdefs/pnames
+function CompositeParameterDef(name::Symbol, comp_path::ComponentPath, pairs::Vector{Pair{T, Symbol}}, kwargs) where T <: AbstractComponentDef
+    # Create the necessary references
+    refs = [UnnamedReference(nameof(comp), param_name) for (comp, param_name) in pairs]
+
+    # Unpack the kwargs
+    datatype = kwargs[:datatype]
+    dim_names = kwargs[:dim_names]
+    description = kwargs[:description]
+    unit = kwargs[:unit]
+    default = kwargs[:default]
+
+    return CompositeParameterDef(name, comp_path, datatype, dim_names, description, unit, default, refs)
+end
+
+# Create a CompositeParameterDef from one subcomponent's ParameterDef (used by import_params!)
+function CompositeParameterDef(obj, param_ref)
+    subcomp_name = param_ref.comp_name
+    pname = param_ref.datum_name
+
+    pardef = obj.namespace[subcomp_name].namespace[pname]
+    return CompositeParameterDef(pname, pathof(obj), pardef.datatype, pardef.dim_names, pardef.description, pardef.unit, pardef.default, [param_ref])
+end
+
+@class CompositeVariableDef <: VariableDef begin
+    ref::UnnamedReference
+end
+
+function CompositeVariableDef(name::Symbol, comp_path::ComponentPath, subcomp::AbstractComponentDef, vname::Symbol)
+    vardef = subcomp.namespace[vname]
+    comp_name = subcomp.name
+    return CompositeVariableDef(name, comp_path, vardef.datatype, vardef.dim_names, vardef.description, vardef.unit, UnnamedReference(comp_name, vname))
+end
+
 # Stores references to the name of a component variable or parameter
 # and the ComponentPath of the component in which it is defined
 @class DatumReference <: NamedObj begin
@@ -113,33 +158,6 @@ end
 
 Base.pathof(dr::AbstractDatumReference) = dr.comp_path
 
-#
-# TBD: rather than store an array of these, perhaps this class should store
-# multiple references, since there is only one name and (at most) one default
-# value. It might look like this:
-#
-# @class DatumReference <: NamedObj
-#
-# struct UnnamedReference
-#    root::AbstractComponentDef
-#    comp_path::ComponentPath
-# end
-#
-# @class ParameterDefReference <: DatumReference begin
-#    name::Symbol is inherited from NamedObj
-#    default::Any    # allows defaults set in composites
-#    refs::Vector{UnnammedReference}
-# end
-#
-# DatumReference would become simpler, with no new fields beyond those in NamedObj,
-# with the root and comp_path moved into VariableDefReference instead.
-#
-# @class VariableDefReference  <: DatumReference begin
-#    name::Symbol is inherited from NamedObj
-#    unnamed_ref::UnnamedReference
-# end
-#
-
 @class ParameterDefReference <: DatumReference begin
     default::Any    # allows defaults set in composites
 end
@@ -156,13 +174,10 @@ function dereference(ref::AbstractDatumReference)
     return comp[ref.name]
 end
 
-# Might not be useful
-# convert(::Type{VariableDef},  ref::VariableDefReference)  = dereference(ref)
-# convert(::Type{ParameterDef}, ref::ParameterDefReference) = dereference(ref)
-
 # Define which types can appear in the namespace dict for leaf and composite compdefs
 global const LeafNamespaceElement      = AbstractDatumDef
-global const CompositeNamespaceElement = Union{AbstractComponentDef, AbstractDatumReference}
+global const CompositeDatumDef         = Union{AbstractCompositeParameterDef, AbstractCompositeVariableDef}
+global const CompositeNamespaceElement = Union{AbstractComponentDef, CompositeDatumDef}
 global const NamespaceElement          = Union{LeafNamespaceElement, CompositeNamespaceElement}
 
 @class mutable CompositeComponentDef <: ComponentDef begin
@@ -171,8 +186,6 @@ global const NamespaceElement          = Union{LeafNamespaceElement, CompositeNa
     # Names of external params that the ConnectorComps will use as their :input2 parameters.
     backups::Vector{Symbol}
 
-    # store default values at the composite level until ModelDef is built
-    defaults::Vector{ParameterDefReference}
 
     sorted_comps::Union{Nothing, Vector{Symbol}}
 
@@ -188,7 +201,6 @@ global const NamespaceElement          = Union{LeafNamespaceElement, CompositeNa
         self.comp_path = ComponentPath(self.name)
         self.internal_param_conns = Vector{InternalParameterConnection}()
         self.backups = Vector{Symbol}()
-        self.defaults = Vector{ParameterDefReference}()
         self.sorted_comps = nothing
     end
 end

From f69689a5b6db644f263c6ea6bf6ab7740cd2fe41 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Wed, 25 Mar 2020 16:53:08 -0400
Subject: [PATCH 02/25] small edits to def functions

---
 src/core/connections.jl | 22 ++++++++++++++++++----
 src/core/defs.jl        |  9 ++++-----
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/core/connections.jl b/src/core/connections.jl
index 546b9478e..b5af0313c 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -361,11 +361,16 @@ end
 """
     connection_refs(obj::AbstractCompositeComponentDef)
 
-Return a vector of UnnamedReference's to parameters with connections.
+Return a vector of UnnamedReference's to parameters from subcomponents that are either found in
+internal connections or that have been already imported in a parameter definition.
 """
 function connection_refs(obj::AbstractCompositeComponentDef)
     refs = UnnamedReference[]
 
+    for conn in obj.internal_param_conns
+        push!(refs, UnnamedReference(conn.dst_comp_path.names[end], conn.dst_par_name))
+    end
+
     for item in values(obj.namespace)
         if item isa CompositeParameterDef
             for ref in item.refs
@@ -374,9 +379,18 @@ function connection_refs(obj::AbstractCompositeComponentDef)
         end
     end
 
-    if obj isa ModelDef
-        append!(refs, [param_ref(obj, epc) for epc in external_param_conns(obj)])
-        error("CK need to implement")
+    return refs
+end
+
+function connection_refs(obj::ModelDef)
+    refs = UnnamedReference[]
+
+    for conn in obj.internal_param_conns
+        push!(refs, UnnamedReference(conn.dst_comp_path.names[end], conn.dst_par_name))
+    end
+
+    for conn in obj.external_param_conns
+        push!(refs, UnnamedReference(conn.comp_path.names[end], conn.param_name))
     end
 
     return refs
diff --git a/src/core/defs.jl b/src/core/defs.jl
index d711d3157..e81729be5 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -512,7 +512,7 @@ end
 function subcomp_params(obj::AbstractCompositeComponentDef)
     params = UnnamedReference[]
     for (name, sub_obj) in obj.namespace
-        if sub_obj isa ComponentDef
+        if sub_obj isa AbstractComponentDef
             for (subname, curr_obj) in sub_obj.namespace
                 if curr_obj isa AbstractParameterDef
                     push!(params, UnnamedReference(name, subname))
@@ -607,11 +607,10 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
     found_comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
 
     if ! has_parameter(md, param_name)
-        count = length(found_comps)
-        if count >= 1
-            import_param!(md, param_name, [comp => param_name for comp in found_comps]...)
-        else # count == 0
+        if isempty(found_comps)
             error("Can't set parameter :$param_name; not found in ModelDef or children")
+        else 
+            import_param!(md, param_name, [comp => param_name for comp in found_comps]...)
         end
     end
 

From c3338fde50d9bce2bb01b388bfc23d5de4ab5a37 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 31 Mar 2020 13:37:52 -0400
Subject: [PATCH 03/25] redo ModelDef's storage of external parameters

---
 src/core/defcomposite.jl | 32 ++++++++++++++++++++------------
 src/core/defs.jl         | 34 +++++++++++++++++++++-------------
 src/core/model.jl        | 25 +++----------------------
 3 files changed, 44 insertions(+), 47 deletions(-)

diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index 217be4924..e2e6a4187 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -226,33 +226,41 @@ function import_params!(obj::AbstractCompositeComponentDef;
     end
 end
 
+function _find_collisions(fields, pairs)
+    collisions = Symbol[]
+
+    pardefs = [comp.namespace[param_name] for (comp, param_name) in pairs]
+    for f in fields
+        subcomponent_set = Set([getproperty(pardef, f) for pardef in pardefs])
+        length(subcomponent_set) > 1 && push!(collisions, f)
+    end
+
+    return collisions
+end
+
 # `kwargs` contains the keywords specified by the user when defining the composite parameter in @defcomposite.
 # If the user does not provide a value for one or any of the possible fields, this function looks at the fields 
 # of the subcomponents' parameters to use, but errors if any of them are in conflict.
 # Note that :dim_names and :datatype can't be specified at the composite level, but must match from the subcomponents.
 function _resolve_composite_parameter_kwargs(obj::AbstractCompositeComponentDef, kwargs::Dict{Symbol, Any}, pairs::Vector{Pair{T, Symbol}}, parname::Symbol)  where T <: AbstractComponentDef
     
-    composite_parameter_conflict_fields = (:default, :description, :unit, :dim_names, :datatype)
-
-    # Access the subcomponents' ParameterDef's specified in `pairs` to be used below for field-checking
-    pardefs = [comp.namespace[param_name] for (comp, param_name) in pairs]
+    fields = (:default, :description, :unit, :dim_names, :datatype)
+    collisions = _find_collisions(fields, pairs)
 
     # Create a new dictionary of resolved values to return
     new_kwargs = Dict{Symbol, Any}()
 
-    for f in composite_parameter_conflict_fields
+    for f in fields
         try 
             new_kwargs[f] = kwargs[f] # Get the user specified value for this field if there is one
         catch e
             # If the composite definition does not specify a value, then need to look to subcomponents and resolve or error
-            subcomponent_set = Set([getproperty(pardef, f) for pardef in pardefs]) # could add an optional "if" here if we want to exclude nothing's or ""'s
-            n = length(subcomponent_set)
-            if n == 1
-                new_kwargs[f] = collect(subcomponent_set)[1]
-            elseif n == 0
-                new_kwargs[f] = nothing
-            else
+            if f in collisions
                 error("Cannot build composite parameter :$parname, subcomponents have conflicting values for the \"$f\" field.")
+            else
+                compdef, curr_parname = pairs[1]
+                pardef = compdef[curr_parname]   
+                new_kwargs[f] = getproperty(pardef, f)
             end
         end
     end
diff --git a/src/core/defs.jl b/src/core/defs.jl
index e81729be5..595095a4e 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -594,23 +594,32 @@ end
 """
     set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
 
-Set the value of a parameter exposed in `md` by following the ParameterDefReference. If
-not found in the local namespace, import it if there is only one such parameter in md'same
-children, and it is not yet bound. Otherwise raise an error.
+Set the value of a parameter in all components of the model that have a parameter of 
+the specified name.
 
 The `value` can by a scalar, an array, or a NamedAray. Optional argument 'dims' is a list
 of the dimension names of the provided data, and will be used to check that they match the
 model's index labels.
 """
-function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
+function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignore_units::Bool=false)
     # search immediate subcomponents for this parameter
     found_comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
 
-    if ! has_parameter(md, param_name)
-        if isempty(found_comps)
-            error("Can't set parameter :$param_name; not found in ModelDef or children")
-        else 
-            import_param!(md, param_name, [comp => param_name for comp in found_comps]...)
+    if isempty(found_comps)
+        error("Can't set parameter :$param_name; not found in ModelDef or children")
+    end
+
+    # which fields to check for collisions in subcomponents
+    fields = ignore_units ? [:dim_names, :datatype] : [:dim_names, :datatype, :unit]
+    collisions = _find_collisions(fields, [comp => param_name for comp in found_comps])
+    if ! isempty(collisions) 
+        if :unit in collisions
+            error("Cannot set parameter :$param_name in the model, components have conflicting values for the :unit field of this parameter. ", 
+            "Call `set_param!` with optional keyword argument `ignore_units = true` to override.")
+        else
+            spec = join(collisions, " and ")
+            error("Cannot set parameter :$param_name in the model, components have conflicting values for the $spec of this parameter. ",
+            "Set these parameters with separate calls to `set_param!(m, comp_name, param_name, unique_param_name, value)`.")
         end
     end
 
@@ -622,9 +631,8 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
         check_parameter_dimensions(md, value, dims, param_name)
     end
 
-    param_def = md[param_name]
-    comp_name = found_comps[1].name
-    comp_def = md[comp_name]
+    comp_def = found_comps[1]   # since we alread checked that the found comps have no conflicting fields in their parameter definitions, we can just use the first one for reference below
+    param_def = comp_def[param_name]
     param_dims = param_def.dim_names
     num_dims = length(param_dims)
 
@@ -641,7 +649,7 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
             value_dims = length(size(value))
             if num_dims != value_dims
                 error("Mismatched data size for a set parameter call: dimension :$param_name",
-                      " in $comp_name has $num_dims dimensions; indicated value",
+                      " in has $num_dims dimensions; indicated value",
                       " has $value_dims dimensions.")
             end
             value = convert(Array{dtype, num_dims}, value)
diff --git a/src/core/model.jl b/src/core/model.jl
index 24ed666fc..bcf7db1e0 100644
--- a/src/core/model.jl
+++ b/src/core/model.jl
@@ -341,33 +341,14 @@ that they match the model's index labels.
 """
 @delegate set_param!(m::Model, comp_name::Symbol, param_name::Symbol, value, dims=nothing) => md
 
-# Deprecated?
-"""
-    set_param!(m::Model, path::AbstractString, param_name::Symbol, value, dims=nothing)
-
-Set a parameter for a component with the given relative path (as a string), in which "/x" means the
-component with name `:x` beneath `m.md`. If the path does not begin with "/", it is treated as
-relative to `m.md`, which at the top of the hierarchy, produces the same result as starting with "/".
-"""
-@delegate set_param!(m::Model, path::AbstractString, param_name::Symbol, value, dims=nothing) => md
-
-# Deprecated?
-"""
-    set_param!(m::Model, path::AbstractString, value, dims=nothing)
-
-Similar to above but param_name appears in `path` after a colon delimiter.
-"""
-@delegate set_param!(m::Model, path::AbstractString, value, dims=nothing) => md
 
 """
     set_param!(m::Model, param_name::Symbol, value, dims=nothing)
 
-Set the value of a parameter exposed in the ModelDef (m.md).
+Set the value of a parameter in all components of the model that have a parameter of 
+the specified name.
 """
-@delegate set_param!(m::Model, param_name::Symbol, value, dims=nothing) => md
-
-# Deprecated?
-@delegate set_param!(m::Model, comp_path::ComponentPath, param_name::Symbol, value, dims=nothing) => md
+@delegate set_param!(m::Model, param_name::Symbol, value, dims=nothing; ignore_units::Bool=false) => md
 
 @delegate import_params!(m::Model) => md
 

From c079e8dbfd276b082c9894ec82e1eb6354311cbe Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 31 Mar 2020 17:44:18 -0400
Subject: [PATCH 04/25] fix setting defaults in build

---
 src/core/build.jl | 41 ++++++-----------------------------------
 1 file changed, 6 insertions(+), 35 deletions(-)

diff --git a/src/core/build.jl b/src/core/build.jl
index b504696c8..756125dda 100644
--- a/src/core/build.jl
+++ b/src/core/build.jl
@@ -188,51 +188,24 @@ function _set_defaults!(md::ModelDef)
     not_set = unconnected_params(md)
     isempty(not_set) && return
 
-    d = Dict()
-
-    function _store_defaults(obj)
-        if obj isa AbstractCompositeComponentDef
-            for ref in obj.defaults
-                d[(pathof(ref), nameof(ref))] = ref.default
-            end
-        else
-            for param in parameters(obj)
-                if param.default !== nothing
-                    d[(pathof(obj), nameof(param))] = param.default
-                end
-            end
-        end
-    end
-    recurse(md, _store_defaults)
-
     for ref in not_set
-        param = dereference(ref)
-        path = pathof(param)
-        name = nameof(param)
-        key = (path, name)
-        value = get(d, key, missing)
-        if value !== missing
-            # @info "Setting param :$name to default $value"
-            set_param!(md, name, value)
-        end
+        comp_name, par_name = ref.comp_name, ref.datum_name
+        pardef = md[comp_name][par_name]
+        default_value = pardef.default
+        default_value === nothing || set_param!(md, par_name, default_value)
     end
 end
 
 function _build(md::ModelDef)
-    # moved to build(m), below
-    # import_params!(md)
 
     # @info "_build(md)"
     add_connector_comps!(md)
 
-    # moved to build(m), below
-    # _set_defaults!(md)
-
     # check if all parameters are set
     not_set = unconnected_params(md)
 
     if ! isempty(not_set)
-        params = join(not_set, "\n  ")
+        params = join([p.datum_name for p in not_set], "\n  ")
         error("Cannot build model; the following parameters are not set:\n  $params")
     end
 
@@ -253,11 +226,9 @@ function _build(md::ModelDef)
 end
 
 function build(m::Model)
-    # import any unconnected params into ModelDef
-    import_params!(m.md)
 
     # apply defaults to unset parameters
-    _set_defaults!(m.md)
+    _set_defaults!(m.md)    # should this actually happen on the deepcopy below isntead?
 
     # Reference a copy in the ModelInstance to avoid changes underfoot
     md = deepcopy(m.md)

From b34af1be2cc9edea3c234f213abae2099691d8e3 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Wed, 1 Apr 2020 15:49:38 -0400
Subject: [PATCH 05/25] Get build function working with new composite datum
 defs

---
 src/core/build.jl       |  19 +++---
 src/core/connections.jl | 139 ++++++++++++++++++++++++----------------
 src/core/defs.jl        |   4 +-
 src/core/types/core.jl  |   3 +
 4 files changed, 97 insertions(+), 68 deletions(-)

diff --git a/src/core/build.jl b/src/core/build.jl
index 756125dda..2bbfe54e6 100644
--- a/src/core/build.jl
+++ b/src/core/build.jl
@@ -104,19 +104,20 @@ function _collect_params(md::ModelDef, var_dict::Dict{ComponentPath, Any})
     pdict = Dict{Tuple{ComponentPath, Symbol}, Any}()
 
     for conn in conns
-        ipc = dereferenced_conn(md, conn)
-        # @info "src_comp_path: $(ipc.src_comp_path)"
-        src_vars = var_dict[ipc.src_comp_path]
-        # @info "src_vars: $src_vars, name: $(ipc.src_var_name)"
-        var_value_obj = get_property_obj(src_vars, ipc.src_var_name)
-        pdict[(ipc.dst_comp_path, ipc.dst_par_name)] = var_value_obj
-        # @info "internal conn: $(ipc.src_comp_path):$(ipc.src_var_name) => $(ipc.dst_comp_path):$(ipc.dst_par_name)"
+        ipcs = _get_leaf_level_ipcs(md, conn)
+        src_vars = var_dict[ipcs[1].src_comp_path]
+        var_value_obj = get_property_obj(src_vars, ipcs[1].src_var_name)
+        for ipc in ipcs
+            pdict[(ipc.dst_comp_path, ipc.dst_par_name)] = var_value_obj
+        end
     end
 
     for epc in external_param_conns(md)
         param = external_param(md, epc.external_param)
-        pdict[(epc.comp_path, epc.param_name)] = (param isa ScalarModelParameter ? param : value(param))
-        # @info "external conn: $(pathof(epc)).$(nameof(epc)) => $(param)"
+        leaf_level_epcs = _get_leaf_level_epcs(md, epc)
+        for leaf_epc in leaf_level_epcs
+            pdict[(leaf_epc.comp_path, leaf_epc.param_name)] = (param isa ScalarModelParameter ? param : value(param))
+        end
     end
 
     # Make the external parameter connections for the hidden ConnectorComps.
diff --git a/src/core/connections.jl b/src/core/connections.jl
index b5af0313c..75fbe2b0c 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -294,69 +294,98 @@ function connect_param!(obj::AbstractCompositeComponentDef, dst::AbstractString,
 end
 
 """
-    param_ref(md::ModelDef, conn::InternalParameterConnection)
+    _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
 
-Return a ParameterDefReference to the parameter indicated in the given
-connection. Used for comparing connections to parameters to find those
-that are (un)connected.
+Recurses through sub components and finds the full path(s) to desired datum, and their
+names at the leaf level. Returns a tuple (paths::Vector{ComponentPath}, datum_names::Vector{Symbol})
 """
-function param_ref(obj::AbstractCompositeComponentDef, conn::InternalParameterConnection)
-    comp = find_comp(obj, conn.dst_comp_path)
-    comp !== nothing || error("Can't find $(conn.dst_comp_path) from $(obj.comp_id)")
-    return datum_reference(comp, conn.dst_par_name)
-end
+function _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
+
+    # Base case-- leaf component
+    if obj isa ComponentDef
+        return ([nothing], [datum_name])
+    end
+
+    datumdef = obj[datum_name]
+    if datumdef isa CompositeVariableDef
+        refs = [datumdef.ref]   # CompositeVariableDef's can only point to one subcomponent
+    else
+        refs = datumdef.refs    # ComposteParameterDef's can have multiple refs
+    end
+
+    paths = []
+    datum_names = []
 
-function param_ref(md::ModelDef, conn::ExternalParameterConnection)
-    comp = find_comp(md, pathof(conn))
-    comp !== nothing || error("Can't find $(pathof(conn)) from $(md.comp_id)")
-    return datum_reference(comp, nameof(conn))
+    for ref in refs
+        # Get the comp and datum's for the current ref
+        next_obj = obj[ref.comp_name]
+        next_datum_name = ref.datum_name
+
+        # Recurse
+        sub_paths, sub_datum_names = _find_paths_and_names(next_obj, next_datum_name)
+
+        # Append the paths, and save with datum_names
+        for (sp, dn) in zip(sub_paths, sub_datum_names)
+            push!(paths, ComponentPath(next_obj.name, sp))
+            push!(datum_names, dn)
+        end
+    end
+
+    return (paths, datum_names)
 end
 
 """
-    dereferenced_conn(obj::AbstractCompositeComponentDef,
-                      conn::InternalParameterConnection)
+    _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
 
-Convert an InternalParameterConnection with datum references to one referring
-only to leaf component pars and vars.
+Returns a vector of InternalParameterConnections that represent all of the connections at the leaf level 
+that need to be made under the hood as specified by `conn`.
 """
-function dereferenced_conn(obj::AbstractCompositeComponentDef,
-                           conn::InternalParameterConnection)
-    comp = find_comp(obj, conn.dst_comp_path)
-    comp !== nothing || error("Can't find $(conn.dst_comp_path) from $(obj.comp_id)")
-    pref = datum_reference(comp, conn.dst_par_name)
+function _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
+
+    top_dst_path = conn.dst_comp_path
+    comp = find_comp(md, top_dst_path)
+    comp !== nothing || error("Can't find $(top_dst_path) from $(md.comp_id)")
+    par_sub_paths, param_names = _find_paths_and_names(comp, conn.dst_par_name)
+    param_paths = [ComponentPath(top_dst_path, sub_path) for sub_path in par_sub_paths]
 
-    comp = find_comp(obj, conn.src_comp_path)
-    comp !== nothing || error("Can't find $(conn.src_comp_path) from $(obj.comp_id)")
-    vref = datum_reference(comp, conn.src_var_name)
+    top_src_path = conn.src_comp_path
+    comp = find_comp(md, top_src_path)
+    comp !== nothing || error("Can't find $(top_src_path) from $(md.comp_id)")
+    var_sub_path, var_name = _find_paths_and_names(comp, conn.src_var_name)
+    var_path = ComponentPath(top_src_path, var_sub_path[1])
 
-    ipc = InternalParameterConnection(pathof(vref), nameof(vref), pathof(pref), nameof(pref),
-                                      conn.ignoreunits, conn.backup; offset=conn.offset)
-    return ipc
+    ipcs = [InternalParameterConnection(var_path, var_name[1], param_path, param_name, 
+        conn.ignoreunits, conn.backup; offset=conn.offset) for (param_path, param_name) in
+        zip(param_paths, param_names)]
+    return ipcs
 end
 
-# """
-#     connection_refs(obj::AbstractCompositeComponentDef)
 
-# Return a vector of ParameterDefReferences to parameters with connections.
-# The references are always to the original Parameter definition in a leaf
-# component.
-# """
-# function connection_refs(obj::AbstractCompositeComponentDef)
-#     refs = ParameterDefReference[]
-#     root = get_root(obj)
-#     # @info "root of $(obj.comp_id) is $(root.comp_id))"
+"""
+    _get_leaf_level_epcs(md::AbstractCompositeComponentDef, epc::ExternalParameterConnection)
+
+Returns a vector that has a new ExternalParameterConnections that represent all of the connections at the leaf level 
+that need to be made under the hood as specified by `epc`.
+"""
+function _get_leaf_level_epcs(md::ModelDef, epc::ExternalParameterConnection)
+
+    comp = find_comp(md, epc.comp_path)
+    comp !== nothing || error("Can't find $(epc.comp_path) from $(md.comp_id)")
+    par_sub_paths, param_names = _find_paths_and_names(comp, epc.param_name)
 
-#     function _add_conns(obj::AbstractCompositeComponentDef)
-#         append!(refs, [param_ref(root, conn) for conn in obj.internal_param_conns])
-#     end
+    leaf_epcs = ExternalParameterConnection[]
+    external_param_name = epc.external_param
 
-#     recurse(obj, _add_conns; composite_only=true)
+    top_path = epc.comp_path
 
-#     if obj isa ModelDef
-#         append!(refs, [param_ref(obj, epc) for epc in external_param_conns(obj)])
-#     end
-#     return refs
-# end
+    for (par_sub_path, param_name) in zip(par_sub_paths, param_names)
+        param_path = ComponentPath(top_path, par_sub_path)
+        epc = ExternalParameterConnection(param_path, param_name, external_param_name)
+        push!(leaf_epcs, epc)
+    end
+
+    return leaf_epcs
+end
 
 """
     connection_refs(obj::AbstractCompositeComponentDef)
@@ -382,6 +411,12 @@ function connection_refs(obj::AbstractCompositeComponentDef)
     return refs
 end
 
+"""
+    connection_refs(obj::ModelDef)
+
+Return a vector of UnnamedReference's to parameters from subcomponents that are either found in
+internal connections or that have been already connected to external parameter values.
+"""
 function connection_refs(obj::ModelDef)
     refs = UnnamedReference[]
 
@@ -396,16 +431,6 @@ function connection_refs(obj::ModelDef)
     return refs
 end
 
-# """
-#     unconnected_params(obj::AbstractCompositeComponentDef)
-
-# Return a list of ParameterDefReferences to parameters that have not been connected
-# to a value.
-# """
-# function unconnected_params(obj::AbstractCompositeComponentDef)
-#     return setdiff(leaf_params(obj), connection_refs(obj))
-# end
-
 """
     unconnected_params(obj::AbstractCompositeComponentDef)
 
diff --git a/src/core/defs.jl b/src/core/defs.jl
index 595095a4e..60d0c3b49 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -700,7 +700,7 @@ end
 #
 variable(obj::ComponentDef, name::Symbol) = _ns_get(obj, name, VariableDef)
 
-variable(obj::AbstractCompositeComponentDef, name::Symbol) = _ns_get(obj, name, VariableDefReference)
+variable(obj::AbstractCompositeComponentDef, name::Symbol) = _ns_get(obj, name, CompositeVariableDef)
 
 variable(comp_id::ComponentId, var_name::Symbol) = variable(compdef(comp_id), var_name)
 
@@ -717,7 +717,7 @@ variable(dr::VariableDefReference) = variable(compdef(dr), nameof(dr))
 
 has_variable(comp_def::ComponentDef, name::Symbol) = _ns_has(comp_def, name, VariableDef)
 
-has_variable(comp_def::AbstractCompositeComponentDef, name::Symbol) = _ns_has(comp_def, name, VariableDefReference)
+has_variable(comp_def::AbstractCompositeComponentDef, name::Symbol) = _ns_has(comp_def, name, CompositeVariableDef)
 
 """
     variable_names(md::AbstractCompositeComponentDef, comp_name::Symbol)
diff --git a/src/core/types/core.jl b/src/core/types/core.jl
index 7d86b8d7f..1e8921170 100644
--- a/src/core/types/core.jl
+++ b/src/core/types/core.jl
@@ -42,10 +42,13 @@ ComponentPath(names::Vector{Symbol}) = ComponentPath(Tuple(names))
 ComponentPath(names::Vararg{Symbol}) = ComponentPath(Tuple(names))
 
 ComponentPath(path::ComponentPath, name::Symbol) = ComponentPath(path.names..., name)
+ComponentPath(name::Symbol, path::ComponentPath) = ComponentPath(name, path.names...)
 
 ComponentPath(path1::ComponentPath, path2::ComponentPath) = ComponentPath(path1.names..., path2.names...)
 
 ComponentPath(::Nothing, name::Symbol) = ComponentPath(name)
+ComponentPath(name::Symbol, ::Nothing) = ComponentPath(name)
+ComponentPath(path::ComponentPath, ::Nothing) = ComponentPath(path.names...)
 
 # Convert path string like "/foo/bar/baz" to ComponentPath(:foo, :bar, :baz)
 ComponentPath(path::AbstractString) = ComponentPath([Symbol(s) for s in split(path, "/") if s != ""]...)

From fe991804dc5ff9c039400bd1da63e94161b9827b Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Wed, 1 Apr 2020 15:50:04 -0400
Subject: [PATCH 06/25] improve some error messages

---
 src/core/defcomposite.jl | 7 +++++--
 src/core/instances.jl    | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index e2e6a4187..29ccf72aa 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -216,8 +216,11 @@ function import_params!(obj::AbstractCompositeComponentDef;
         end
     end
 
-    unique_names = Set([ref.datum_name for ref in unconn])
-    length(unique_names) == length(unconn) || error("There are unresolved parameter name collisions from subcomponents.")
+    all_names = [ref.datum_name for ref in unconn]
+    unique_names = unique(all_names)
+    _map = Dict([name => count(isequal(name), all_names) for name in unique_names])
+    non_unique = [name for (name, val) in _map if val>1]
+    isempty(non_unique) || error("There are unresolved parameter name collisions from subcomponents for the following parameter names: $(non_unique...).")
 
     for param_ref in unconn
         name = param_ref.datum_name
diff --git a/src/core/instances.jl b/src/core/instances.jl
index 911bdc524..b5fa810de 100644
--- a/src/core/instances.jl
+++ b/src/core/instances.jl
@@ -67,7 +67,7 @@ end
     if T <: ScalarModelParameter
         return setproperty!(prop_obj, :value, value)
     else
-        error("You cannot override indexed variable $name::$T.")
+        error("You cannot override indexed variable $name::$T. Make sure you are using proper indexing syntax in the `run_timestep` function: v.varname[t] = ...")
     end
 end
 

From 048f7cabba49cc164f7ced56e8c0d82a751c0333 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Wed, 1 Apr 2020 18:42:53 -0400
Subject: [PATCH 07/25] fix some outdated tests

---
 src/core/connections.jl                       |  7 ++--
 src/core/defs.jl                              | 21 +++-------
 test/test_main.jl                             | 30 ++++++++------
 test/test_main_variabletimestep.jl            | 31 +++++++-------
 test/test_marginal_models.jl                  |  4 +-
 test/test_model_structure.jl                  |  3 +-
 test/test_model_structure_variabletimestep.jl |  3 +-
 test/test_references.jl                       | 41 -------------------
 8 files changed, 48 insertions(+), 92 deletions(-)
 delete mode 100644 test/test_references.jl

diff --git a/src/core/connections.jl b/src/core/connections.jl
index 75fbe2b0c..de277431a 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -450,10 +450,9 @@ the dictionary keys are strings that match the names of unset parameters in the
 """
 function set_leftover_params!(md::ModelDef, parameters::Dict{T, Any}) where T
     for param_ref in unconnected_params(md)
-        comp_path = pathof(param_ref)
-        param_name = nameof(param_ref)
-        comp_def = compdef(md, comp_path)
-        comp_name = nameof(comp_def)
+        param_name = param_ref.datum_name
+        comp_name = param_ref.comp_name
+        comp_def = find_comp(md, comp_name)
 
         # @info "set_leftover_params: comp_name=$comp_name, param=$param_name"
         # check whether we need to set the external parameter
diff --git a/src/core/defs.jl b/src/core/defs.jl
index 60d0c3b49..fa1782e57 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -364,6 +364,7 @@ parameter(dr::ParameterDefReference) = parameter(compdef(dr), nameof(dr))
 #               name::Symbol) = _ns_has(comp_def, name, ParameterDefReference)
 
 has_parameter(comp_def::AbstractComponentDef, name::Symbol) = _ns_has(comp_def, name, AbstractParameterDef)
+has_parameter(md::ModelDef, name::Symbol) = haskey(md.external_params, name)
 
 function parameter_unit(obj::AbstractComponentDef, param_name::Symbol)
     param = parameter(obj, param_name)
@@ -565,27 +566,15 @@ function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, value,
     set_param!(md, comp_def, param_name, value, dims)
 end
 
-"""
-Compare two items, each of which must be either a ParameterDef or ParameterDefReference,
-to see if they refer to the same ultimate parameter.
-"""
-function _is_same_param(p1::Union{ParameterDef, ParameterDefReference},
-                        p2::Union{ParameterDef, ParameterDefReference})
-    p1 = (p1 isa ParameterDefReference ? dereference(p1) : p1)
-    p2 = (p2 isa ParameterDefReference ? dereference(p2) : p2)
-    return p1 == p2
-end
-
 function set_param!(md::ModelDef, comp_def::AbstractComponentDef, param_name::Symbol,
                     value, dims=nothing)
     has_parameter(comp_def, param_name) ||
         error("Can't find parameter :$param_name in component $(pathof(comp_def))")
 
-    if ! has_parameter(md, param_name)
-        import_param!(md, param_name, comp_def => param_name)
-
-    elseif ! _is_same_param(md[param_name], comp_def[param_name])
-        error("Can't import parameter :$param_name; ModelDef has another item with this name")
+    if has_parameter(md, param_name)
+        error("Cannot set parameter :$param_name, the model already has an external parameter with this name.", 
+        " Use `update_param(m, param_name, value)` to change the value, or use ",
+        "`set_param(m, comp_name, param_name, unique_param_name, value)` to set a value for only this component.")
     end
 
     set_param!(md, param_name, value, dims)
diff --git a/test/test_main.jl b/test/test_main.jl
index b33a00493..6e1a0e9e1 100644
--- a/test/test_main.jl
+++ b/test/test_main.jl
@@ -26,18 +26,22 @@ import Mimi:
 end
 
 
-@defmodel x1 begin
-    index[index1] = [:r1, :r2, :r3]
-    index[time] = 2010:10:2030
-    index[idx3] = 1:3
-    index[idx4] = 1:4
-    component(foo1)
-
-    foo1.par1 = 5.0
-end
-
-# x1 = foo1(Float64, Dict{Symbol, Int}(:time=>10, :index1=>3))
-# x1 = foo1(Float64, Val{1}, Val{1}, Val{10}, Val{1}, Val{1}, Val{1}, Val{1}, Dict{Symbol, Int}(:time=>10, :index1=>3))
+# @defmodel x1 begin
+#     index[index1] = [:r1, :r2, :r3]
+#     index[time] = 2010:10:2030
+#     index[idx3] = 1:3
+#     index[idx4] = 1:4
+#     component(foo1)
+
+#     foo1.par1 = 5.0
+# end
+x1 = Model()
+set_dimension!(x1, :index1, [:r1, :r2, :r3])
+set_dimension!(x1, :time, 2010:10:2030)
+set_dimension!(x1, :idx3, 1:3)
+set_dimension!(x1, :idx4, 1:4)
+add_comp!(x1, foo1)
+set_param!(x1, :foo1, :par1, 5.0)
 
 @test length(dimension(x1.md, :index1)) == 3
 
@@ -46,7 +50,7 @@ end
 par1 = external_param(x1, :par1)
 @test par1.value == 5.0
 
-set_param!(x1, :foo1, :par1, 6.0)
+update_param!(x1, :par1, 6.0)
 par1 = external_param(x1, :par1)
 @test par1.value == 6.0
 
diff --git a/test/test_main_variabletimestep.jl b/test/test_main_variabletimestep.jl
index c374e95c2..9fb3fcf16 100644
--- a/test/test_main_variabletimestep.jl
+++ b/test/test_main_variabletimestep.jl
@@ -25,19 +25,22 @@ import Mimi:
     var5 = Variable(index=[index1, idx4])
 end
 
-@defmodel x1 begin
-    index[index1] = [:r1, :r2, :r3]
-    index[time] = [2010, 2015, 2030]
-    index[idx3] = 1:3
-    index[idx4] = 1:4
-    component(foo1)
-
-    foo1.par1 = 5.0
-    # foo1.par2 = []
-end
-
-# x1 = foo1(Float64, Dict{Symbol, Int}(:time=>10, :index1=>3))
-# x1 = foo1(Float64, Val{1}, Val{1}, Val{10}, Val{1}, Val{1}, Val{1}, Val{1}, Dict{Symbol, Int}(:time=>10, :index1=>3))
+# @defmodel x1 begin
+#     index[index1] = [:r1, :r2, :r3]
+#     index[time] = [2010, 2015, 2030]
+#     index[idx3] = 1:3
+#     index[idx4] = 1:4
+#     component(foo1)
+
+#     foo1.par1 = 5.0
+# end
+x1 = Model()
+set_dimension!(x1, :index1, [:r1, :r2, :r3])
+set_dimension!(x1, :time, [2010, 2015, 2030])
+set_dimension!(x1, :idx3, 1:3)
+set_dimension!(x1, :idx4, 1:4)
+add_comp!(x1, foo1)
+set_param!(x1, :foo1, :par1, 5.0)
 
 @test length(dimension(x1.md, :index1)) == 3
 
@@ -46,7 +49,7 @@ end
 par1 = external_param(x1, :par1)
 @test par1.value == 5.0
 
-set_param!(x1, :foo1, :par1, 6.0)
+update_param!(x1, :par1, 6.0)
 par1 = external_param(x1, :par1)
 @test par1.value == 6.0
 
diff --git a/test/test_marginal_models.jl b/test/test_marginal_models.jl
index e730e9d7f..d40ab3512 100644
--- a/test/test_marginal_models.jl
+++ b/test/test_marginal_models.jl
@@ -23,7 +23,7 @@ set_param!(model1, :compA, :parA, x1)
 mm = MarginalModel(model1, .5)
 
 model2 = mm.marginal
-set_param!(model2, :compA, :parA, x2)
+update_param!(model2, :parA, x2)
 
 run(mm)
 
@@ -34,7 +34,7 @@ end
 mm2 = create_marginal_model(model1, 0.5)
 
 mm2_marginal = mm2.marginal
-set_param!(mm2_marginal, :compA, :parA, x2)
+update_param!(mm2_marginal, :parA, x2)
 
 run(mm2)
 
diff --git a/test/test_model_structure.jl b/test/test_model_structure.jl
index cba4c7f81..bdedb42b2 100644
--- a/test/test_model_structure.jl
+++ b/test/test_model_structure.jl
@@ -59,7 +59,8 @@ unconns = unconnected_params(m)
 @test length(unconns) == 1
 c = compdef(m, :C)
 uconn = unconns[1]
-@test (pathof(uconn), nameof(uconn)) == (c.comp_path, :parC)
+@test uconn.comp_name == :C
+@test uconn.datum_name == :parC
 
 connect_param!(m, :C => :parC, :B => :varB)
 
diff --git a/test/test_model_structure_variabletimestep.jl b/test/test_model_structure_variabletimestep.jl
index da9429f70..3c4ac70d3 100644
--- a/test/test_model_structure_variabletimestep.jl
+++ b/test/test_model_structure_variabletimestep.jl
@@ -67,7 +67,8 @@ unconns = unconnected_params(m)
 @test length(unconns) == 1
 c = compdef(m, :C)
 uconn = unconns[1]
-@test (pathof(uconn), nameof(uconn)) == (c.comp_path, :parC)
+@test uconn.comp_name == :C
+@test uconn.datum_name == :parC
 
 connect_param!(m, :C => :parC, :B => :varB)
 
diff --git a/test/test_references.jl b/test/test_references.jl
deleted file mode 100644
index 7a753321b..000000000
--- a/test/test_references.jl
+++ /dev/null
@@ -1,41 +0,0 @@
-module TestReferences
-
-using Test
-using Mimi
-
-import Mimi:
-    getproperty, @defmodel
-
-@defcomp Foo begin
-    input = Parameter()
-    intermed = Variable(index=[time])
-    
-    function run_timestep(p, v, d, t)
-        v.intermed[t] = p.input
-    end
-end
-    
-@defcomp Bar begin
-    intermed = Parameter(index=[time])
-    output = Variable(index=[time])
-    
-    function run_timestep(p, v, d, t)
-        v.output[t] = p.intermed[t]
-    end
-end
-
-
-@defmodel m begin
-    index[time] = [1, 2]
-    component(Foo)
-    component(Bar)
-
-    Foo.input = 3.14
-    Foo.intermed => Bar.intermed
-end
-
-run(m)
-
-@test m[:Bar, :output][1] == 3.14
-
-end
\ No newline at end of file

From aea85c0e5e9cbbd46e5cd1b2d1882b1459f03129 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 2 Apr 2020 13:24:10 -0400
Subject: [PATCH 08/25] implement other set_param! methods

---
 src/core/defcomposite.jl |  2 +-
 src/core/defs.jl         | 50 ++++++++++++++++++++--------------------
 src/core/model.jl        |  2 +-
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index 29ccf72aa..8d1feccf5 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -224,7 +224,7 @@ function import_params!(obj::AbstractCompositeComponentDef;
 
     for param_ref in unconn
         name = param_ref.datum_name
-        haskey(obj, name) && error("Failed to auto-import parameter :$name from component :$(param_ref.comp_name), this name has already been defined in the Composite component's namesapce.")
+        haskey(obj, name) && error("Failed to auto-import parameter :$name from component :$(param_ref.comp_name), this name has already been defined in the composite component's namespace.")
         obj[name] = CompositeParameterDef(obj, param_ref)
     end
 end
diff --git a/src/core/defs.jl b/src/core/defs.jl
index fa1782e57..335e5ea2a 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -550,34 +550,28 @@ function set_param!(md::ModelDef, comp_name::Symbol, value_dict::Dict{Symbol, An
     set_param!(md, comp_name, param_name, value, dims)
 end
 
-# May be deprecated
-function set_param!(md::ModelDef, comp_path::ComponentPath,
-                    param_name::Symbol, value, dims=nothing)
-    # @info "set_param!($(md.comp_id), $comp_path, $param_name, $value)"
-    comp_def = find_comp(md, comp_path)
-    @or(comp_def, error("Component with path $comp_path not found"))
-    # set_param!(comp.parent, nameof(comp), param_name, value, dims)
-    set_param!(md, comp_def, param_name, value, dims)
+function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, value, dims=nothing)
+    set_param!(md, comp_name, param_name, param_name, value, dims)
 end
 
-function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, value, dims=nothing)
+function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, ext_param_name::Symbol, value, dims=nothing)
     comp_def = compdef(md, comp_name)
     @or(comp_def, error("Top-level component with name $comp_name not found"))
-    set_param!(md, comp_def, param_name, value, dims)
+    set_param!(md, comp_def, param_name, ext_param_name, value, dims)
 end
 
 function set_param!(md::ModelDef, comp_def::AbstractComponentDef, param_name::Symbol,
-                    value, dims=nothing)
+                    ext_param_name::Symbol, value, dims=nothing)
     has_parameter(comp_def, param_name) ||
         error("Can't find parameter :$param_name in component $(pathof(comp_def))")
 
-    if has_parameter(md, param_name)
-        error("Cannot set parameter :$param_name, the model already has an external parameter with this name.", 
+    if has_parameter(md, ext_param_name)
+        error("Cannot set parameter :$ext_param_name, the model already has an external parameter with this name.", 
         " Use `update_param(m, param_name, value)` to change the value, or use ",
         "`set_param(m, comp_name, param_name, unique_param_name, value)` to set a value for only this component.")
     end
 
-    set_param!(md, param_name, value, dims)
+    set_param!(md, param_name, value, dims, comps = [comp_def], ext_param_name = ext_param_name)
 end
 
 """
@@ -590,21 +584,27 @@ The `value` can by a scalar, an array, or a NamedAray. Optional argument 'dims'
 of the dimension names of the provided data, and will be used to check that they match the
 model's index labels.
 """
-function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignore_units::Bool=false)
+function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignoreunits::Bool=false, comps=nothing, ext_param_name=nothing)
     # search immediate subcomponents for this parameter
-    found_comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
+    if comps === nothing
+        comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
+    end
+
+    if ext_param_name === nothing
+        ext_param_name = param_name
+    end
 
-    if isempty(found_comps)
+    if isempty(comps)
         error("Can't set parameter :$param_name; not found in ModelDef or children")
     end
 
     # which fields to check for collisions in subcomponents
-    fields = ignore_units ? [:dim_names, :datatype] : [:dim_names, :datatype, :unit]
-    collisions = _find_collisions(fields, [comp => param_name for comp in found_comps])
+    fields = ignoreunits ? [:dim_names, :datatype] : [:dim_names, :datatype, :unit]
+    collisions = _find_collisions(fields, [comp => param_name for comp in comps])
     if ! isempty(collisions) 
         if :unit in collisions
             error("Cannot set parameter :$param_name in the model, components have conflicting values for the :unit field of this parameter. ", 
-            "Call `set_param!` with optional keyword argument `ignore_units = true` to override.")
+            "Call `set_param!` with optional keyword argument `ignoreunits = true` to override.")
         else
             spec = join(collisions, " and ")
             error("Cannot set parameter :$param_name in the model, components have conflicting values for the $spec of this parameter. ",
@@ -620,7 +620,7 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignor
         check_parameter_dimensions(md, value, dims, param_name)
     end
 
-    comp_def = found_comps[1]   # since we alread checked that the found comps have no conflicting fields in their parameter definitions, we can just use the first one for reference below
+    comp_def = comps[1]   # since we alread checked that the found comps have no conflicting fields in their parameter definitions, we can just use the first one for reference below
     param_def = comp_def[param_name]
     param_dims = param_def.dim_names
     num_dims = length(param_dims)
@@ -670,16 +670,16 @@ function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignor
             values = value
         end
 
-        set_external_array_param!(md, param_name, values, param_dims)
+        set_external_array_param!(md, ext_param_name, values, param_dims)
 
     else # scalar parameter case
         value = convert(dtype, value)
-        set_external_scalar_param!(md, param_name, value)
+        set_external_scalar_param!(md, ext_param_name, value)
     end
 
     # connect_param! calls dirty! so we don't have to
-    for comp in found_comps
-        connect_param!(md, comp, param_name, param_name)
+    for comp in comps
+        connect_param!(md, comp, param_name, ext_param_name)
     end
     nothing
 end
diff --git a/src/core/model.jl b/src/core/model.jl
index bcf7db1e0..5fae0a22b 100644
--- a/src/core/model.jl
+++ b/src/core/model.jl
@@ -348,7 +348,7 @@ that they match the model's index labels.
 Set the value of a parameter in all components of the model that have a parameter of 
 the specified name.
 """
-@delegate set_param!(m::Model, param_name::Symbol, value, dims=nothing; ignore_units::Bool=false) => md
+@delegate set_param!(m::Model, param_name::Symbol, value, dims=nothing; ignoreunits::Bool=false) => md
 
 @delegate import_params!(m::Model) => md
 

From 755fef7c6efd2def43cd587740b73814af335a19 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 2 Apr 2020 13:24:27 -0400
Subject: [PATCH 09/25] update set_leftover_params function

---
 src/core/connections.jl | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/core/connections.jl b/src/core/connections.jl
index de277431a..644f33214 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -453,17 +453,25 @@ function set_leftover_params!(md::ModelDef, parameters::Dict{T, Any}) where T
         param_name = param_ref.datum_name
         comp_name = param_ref.comp_name
         comp_def = find_comp(md, comp_name)
+        param_def = comp_def[param_name]
+
 
         # @info "set_leftover_params: comp_name=$comp_name, param=$param_name"
         # check whether we need to set the external parameter
+        _skip = false
         if external_param(md, param_name, missing_ok=true) === nothing
-            value = parameters[string(param_name)]
-            param_dims = parameter_dimensions(md, comp_name, param_name)
-
-            set_external_param!(md, param_name, value; param_dims = param_dims)
+            if haskey(parameters, string(param_name))  
+                value = parameters[string(param_name)]
+                param_dims = parameter_dimensions(md, comp_name, param_name)
 
+                set_external_param!(md, param_name, value; param_dims = param_dims)
+            elseif param_def.default != nothing
+                _skip = true
+            else
+                error("Cannot set parameter :$param_name, not found in provided dictionary and no default value deteceted.")
+            end
         end
-        connect_param!(md, comp_name, param_name, param_name)
+        _skip || connect_param!(md, comp_name, param_name, param_name)
     end
     nothing
 end

From 472475c85d2e84399b2915e1af5dde2f74bf1ddb Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 2 Apr 2020 13:26:10 -0400
Subject: [PATCH 10/25] Start new test file

---
 test/runtests.jl                  |   6 +-
 test/test_composite_parameters.jl | 133 ++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 3 deletions(-)
 create mode 100644 test/test_composite_parameters.jl

diff --git a/test/runtests.jl b/test/runtests.jl
index ae8003585..90b4f16cd 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -20,6 +20,9 @@ Electron.prep_test_env()
     @info("test_composite.jl")
     @time include("test_composite.jl")
 
+    @info("test_composite_parameters.jl")
+    @time include("test_composite_parameters.jl")
+
     @info("test_main_variabletimestep.jl")
     @time include("test_main_variabletimestep.jl")
 
@@ -32,9 +35,6 @@ Electron.prep_test_env()
     @info("test_metainfo_variabletimestep.jl")
     @time include("test_metainfo_variabletimestep.jl")
 
-    @info("test_references.jl")
-    @time include("test_references.jl")
-
     @info("test_units.jl")
     @time include("test_units.jl")
 
diff --git a/test/test_composite_parameters.jl b/test/test_composite_parameters.jl
new file mode 100644
index 000000000..25e9253b0
--- /dev/null
+++ b/test/test_composite_parameters.jl
@@ -0,0 +1,133 @@
+using Mimi
+using Test 
+
+@defcomp A begin
+    p1 = Parameter(unit = "\$", default=3)
+    p2 = Parameter()
+
+    v1 = Variable()
+    v2 = Variable(index=[time])
+end
+
+@defcomp B begin
+    p1 = Parameter(unit = "thous \$", default=3)
+    p3 = Parameter(description="B p3")
+    p4 = Parameter(index=[time])
+
+end
+
+#------------------------------------------------------------------------------
+# Test a failure to resolve namespace collisions
+
+fail_expr1 = :(
+    @defcomposite TestFailComposite begin
+        Component(A)
+        Component(B)
+        # Will fail because you need to resolve the namespace collision of the p1's
+    end
+)
+
+err1 = try eval(fail_expr1) catch err err end 
+@test occursin("unresolved parameter name collisions from subcomponents", sprint(showerror, err1))
+
+
+#------------------------------------------------------------------------------
+# Test a failure to resolve conflicting "description" fields
+
+fail_expr2 = :(
+    @defcomposite TestFailComposite begin
+        Component(A)
+        Component(B)
+        
+        p1 = Parameter(A.p1, B.p1, unit="")
+        p2 = Parameter(A.p2, B.p3)  # Will fail because the conflicting descriptions aren't resolved
+    end
+)
+
+err2 = try eval(fail_expr2) catch err err end 
+@test occursin("subcomponents have conflicting values for the \"description\" field", sprint(showerror, err2))
+
+
+#------------------------------------------------------------------------------
+# Test trying to join parameters with different dimensions
+
+fail_expr3 = :(
+    @defcomposite TestFailComposite begin
+        Component(A)
+        Component(B)
+        
+        p1 = Parameter(A.p1, B.p1, unit="")
+        p2 = Parameter(A.p2, B.p4)  # Will fail because different dimensions
+    end
+)
+
+err3 = try eval(fail_expr3) catch err err end 
+@test occursin("subcomponents have conflicting values for the \"dim_names\" field", sprint(showerror, err3))
+
+
+#------------------------------------------------------------------------------
+# Test a failure to auto-import a paramter because it's name has already been used
+
+fail_expr4 = :(
+    @defcomposite TestFailComposite begin
+        Component(A)
+        Component(B)
+        
+        p3 = Parameter(A.p1, B.p1, unit="")    # should fail on auto import of other p3 parameter
+    end
+)
+
+err4 = try eval(fail_expr4) catch err err end 
+@test occursin("this name has already been defined in the composite component's namespace", sprint(showerror, err4))
+
+
+#------------------------------------------------------------------------------
+# Test an attempt to import a parameter twice
+
+fail_expr5 = :(
+    @defcomposite TestFailComposite begin
+        Component(A)
+        Component(B)
+        
+        p1 = Parameter(A.p1, B.p1, unit="") 
+        p1_repeat = Parameter(B.p1) # should not allow a second import of B.p1 (already connected)
+    end
+)
+
+err5 = try eval(fail_expr5) catch err err end 
+@test occursin("Duplicate import", sprint(showerror, err5))
+
+
+#------------------------------------------------------------------------------
+# Test set_param! with unit collision
+
+function get_model()
+    m = Model()
+    set_dimension!(m, :time, 10)
+    add_comp!(m, A)
+    add_comp!(m, B)
+    return m
+end
+
+m1 = get_model()
+err6 = try set_param!(m1, :p1, 5) catch err err end
+@test occursin("components have conflicting values for the :unit field of this parameter", sprint(showerror, err6))
+
+# use ignoreunits flag
+set_param!(m1, :p1, 5, ignoreunits=true)    
+
+err7 = try run(m1) catch err err end
+@test occursin("Cannot build model; the following parameters are not set", sprint(showerror, err7))
+
+# Set separate values for p1 in A and B
+m2 = get_model()
+set_param!(m2, :A, :p1, 1)  # Set the value only for component A
+@test length(m2.md.external_param_conns) == 1 # test that only one connection has been made
+@test Mimi.UnnamedReference(:B, :p1) in Mimi.unconnected_params(m2.md)  # and that B.p1 is still unconnected 
+
+err8 = try set_param!(m2, :B, :p1, 2) catch err err end
+@test occursin("the model already has an external parameter with this name", sprint(showerror, err8))
+
+set_param!(m2, :B, :p1, :B_p1, 2)   # Use a unique name to set B.p1
+@test length(m2.md.external_param_conns) == 2 
+@test Set(keys(m2.md.external_params)) == Set([:p1, :B_p1])

From fa39d65d623f09d3aaf4bb2134e255b04abd5230 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 7 Apr 2020 11:21:34 -0400
Subject: [PATCH 11/25] get ComponentReferences working again

---
 src/core/references.jl  | 12 +++++++++---
 test/runtests.jl        |  3 +++
 test/test_references.jl | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 test/test_references.jl

diff --git a/src/core/references.jl b/src/core/references.jl
index 275281045..69264b353 100644
--- a/src/core/references.jl
+++ b/src/core/references.jl
@@ -2,18 +2,24 @@
     set_param!(ref::ComponentReference, name::Symbol, value)
 
 Set a component parameter as `set_param!(reference, name, value)`.
+This creates a unique name :compname_paramname in the model's external parameter list, 
+and sets the parameter only in the referenced component to that value.
 """
 function set_param!(ref::ComponentReference, name::Symbol, value)
-    set_param!(parent(ref), pathof(ref), name, value)
+    compdef = find_comp(ref)
+    unique_name = Symbol("$(compdef.name)_$name")
+    set_param!(parent(ref), compdef, name, unique_name, value)
 end
 
 """
     Base.setindex!(ref::ComponentReference, value, name::Symbol)
 
-Set a component parameter as `reference[symbol] = value`.
+Set a component parameter as `reference[name] = value`.
+This creates a unique name :compname_paramname in the model's external parameter list, 
+and sets the parameter only in the referenced component to that value.
 """
 function Base.setindex!(ref::ComponentReference, value, name::Symbol)
-    set_param!(parent(ref), pathof(ref), name, value)
+    set_param!(ref, name, value)
 end
 
 """
diff --git a/test/runtests.jl b/test/runtests.jl
index 90b4f16cd..79901aa3b 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -35,6 +35,9 @@ Electron.prep_test_env()
     @info("test_metainfo_variabletimestep.jl")
     @time include("test_metainfo_variabletimestep.jl")
 
+    @info("test_references.jl")
+    @time include("test_references.jl")
+
     @info("test_units.jl")
     @time include("test_units.jl")
 
diff --git a/test/test_references.jl b/test/test_references.jl
new file mode 100644
index 000000000..7b13b0595
--- /dev/null
+++ b/test/test_references.jl
@@ -0,0 +1,41 @@
+module TestReferences
+
+using Test
+using Mimi
+
+@defcomp A begin
+    p1 = Parameter()
+    v1 = Variable(index = [time])
+    function run_timestep(p, v, d, t)
+        v.v1[t] = gettime(t)
+    end
+end
+
+@defcomp B begin
+    p1 = Parameter()
+    p2 = Parameter(index = [time])
+end
+
+m = Model()
+set_dimension!(m, :time, 10)
+refA = add_comp!(m, A, :foo)
+refB = add_comp!(m, B)
+
+refA[:p1] = 3   # creates a parameter specific to this component, with name "foo_p1"
+@test length(m.md.external_param_conns) == 1
+@test Mimi.UnnamedReference(:B, :p1) in Mimi.unconnected_params(m.md)
+@test :foo_p1 in keys(m.md.external_params)
+
+refB[:p1] = 5
+@test length(m.md.external_param_conns) == 2
+@test :B_p1 in keys(m.md.external_params)
+
+# Use the ComponentReferences to make an internal connection
+refB[:p2] = refA[:v1]
+
+run(m)
+@test m[:foo, :p1] == 3
+@test m[:B, :p1] == 5
+@test m[:B, :p2] == collect(1:10)
+
+end
\ No newline at end of file

From e10d64338cb06e0aaeae3335b31a92e5d720ce2d Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 7 Apr 2020 11:26:21 -0400
Subject: [PATCH 12/25] simplify tools test file

---
 test/test_tools.jl | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/test/test_tools.jl b/test/test_tools.jl
index ddcd1b50b..c61ce04da 100644
--- a/test/test_tools.jl
+++ b/test/test_tools.jl
@@ -13,40 +13,9 @@ import Mimi:
 @test pretty_string("_snake__case__weird_") == pretty_string(:_snake__case__weird_) == "Snake Case Weird"
 
 #utils: interpolate
-stepsize = 2       # N.B. ERROR: cannot assign variable Base.step from module Main
-final = 10         # N.B. ERROR: cannot assign variable Base.last from module Main
+stepsize = 2       
+final = 10         
 ts = 10
 @test Mimi.interpolate(collect(0:stepsize:final), ts) == collect(0:stepsize/ts:final)
 
-@defcomp Foo begin
-    input = Parameter()
-    intermed = Variable(index=[time])
-
-    function run_timestep(p, v, d, t)
-        v.intermed[t] = p.input
-    end
-end
-
-@defcomp Bar begin
-    intermed = Parameter(index=[time])
-    output = Variable(index=[time])
-
-    function run_timestep(p, v, d, t)
-        v.output[t] = p.intermed[t]
-    end
-end
-
-m = Model()
-set_dimension!(m, :time, 2)
-foo = add_comp!(m, Foo)
-bar = add_comp!(m, Bar)
-
-foo[:input] = 3.14
-
-# The connection via references is broken...
-# bar[:intermed] = m.md[:Foo][:intermed]
-connect_param!(m, :Bar, :intermed,  :Foo, :intermed)
-
-run(m)
-
 end #module

From 5689fd2583d67237245b4e5d22b9fea8c91adacf Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 7 Apr 2020 13:58:19 -0400
Subject: [PATCH 13/25] make `dims` a keyword argument in set_param!

---
 src/core/defs.jl                  | 25 +++++++++----------
 src/core/model.jl                 | 21 ++++++++++++----
 test/test_composite_parameters.jl |  4 +++
 test/test_parameter_labels.jl     |  2 +-
 test/test_parametertypes.jl       | 41 +++++++++++++++++++++++++++++++
 5 files changed, 74 insertions(+), 19 deletions(-)

diff --git a/src/core/defs.jl b/src/core/defs.jl
index 335e5ea2a..25994c048 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -539,29 +539,28 @@ end
 
 """
     set_param!(md::ModelDef, comp_path::ComponentPath, param_name::Symbol,
-               value_dict::Dict{Symbol, Any}, dims=nothing)
+               value_dict::Dict{Symbol, Any}; dims=nothing)
 
 Call `set_param!()` with `param_name` and a value dict in which `value_dict[param_name]` references
 the value of parameter `param_name`.
 """
 function set_param!(md::ModelDef, comp_name::Symbol, value_dict::Dict{Symbol, Any},
-                    param_name::Symbol, dims=nothing)
+                    param_name::Symbol; dims=nothing)
     value = value_dict[param_name]
-    set_param!(md, comp_name, param_name, value, dims)
+    set_param!(md, comp_name, param_name, value, dims=dims)
 end
 
-function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, value, dims=nothing)
-    set_param!(md, comp_name, param_name, param_name, value, dims)
+function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, value; dims=nothing)
+    set_param!(md, comp_name, param_name, param_name, value, dims=dims)
 end
 
-function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, ext_param_name::Symbol, value, dims=nothing)
+function set_param!(md::ModelDef, comp_name::Symbol, param_name::Symbol, ext_param_name::Symbol, value; dims=nothing)
     comp_def = compdef(md, comp_name)
     @or(comp_def, error("Top-level component with name $comp_name not found"))
-    set_param!(md, comp_def, param_name, ext_param_name, value, dims)
+    set_param!(md, comp_def, param_name, ext_param_name, value, dims=dims)
 end
 
-function set_param!(md::ModelDef, comp_def::AbstractComponentDef, param_name::Symbol,
-                    ext_param_name::Symbol, value, dims=nothing)
+function set_param!(md::ModelDef, comp_def::AbstractComponentDef, param_name::Symbol, ext_param_name::Symbol, value; dims=nothing)
     has_parameter(comp_def, param_name) ||
         error("Can't find parameter :$param_name in component $(pathof(comp_def))")
 
@@ -571,20 +570,20 @@ function set_param!(md::ModelDef, comp_def::AbstractComponentDef, param_name::Sy
         "`set_param(m, comp_name, param_name, unique_param_name, value)` to set a value for only this component.")
     end
 
-    set_param!(md, param_name, value, dims, comps = [comp_def], ext_param_name = ext_param_name)
+    set_param!(md, param_name, value, dims = dims, comps = [comp_def], ext_param_name = ext_param_name)
 end
 
 """
-    set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing)
+    set_param!(md::ModelDef, param_name::Symbol, value; dims=nothing)
 
 Set the value of a parameter in all components of the model that have a parameter of 
 the specified name.
 
-The `value` can by a scalar, an array, or a NamedAray. Optional argument 'dims' is a list
+The `value` can by a scalar, an array, or a NamedAray. Optional keyword argument 'dims' is a list
 of the dimension names of the provided data, and will be used to check that they match the
 model's index labels.
 """
-function set_param!(md::ModelDef, param_name::Symbol, value, dims=nothing; ignoreunits::Bool=false, comps=nothing, ext_param_name=nothing)
+function set_param!(md::ModelDef, param_name::Symbol, value; dims=nothing, ignoreunits::Bool=false, comps=nothing, ext_param_name=nothing)
     # search immediate subcomponents for this parameter
     if comps === nothing
         comps = [comp for (compname, comp) in components(md) if has_parameter(comp, param_name)]
diff --git a/src/core/model.jl b/src/core/model.jl
index 5fae0a22b..6282cdba8 100644
--- a/src/core/model.jl
+++ b/src/core/model.jl
@@ -332,23 +332,34 @@ Delete a `component`` by name from a model `m`'s ModelDef, and nullify the Model
 @delegate Base.delete!(m::Model, comp_name::Symbol) => md
 
 """
-    set_param!(m::Model, comp_name::Symbol, name::Symbol, value, dims=nothing)
+    set_param!(m::Model, comp_name::Symbol, param_name::Symbol, value; dims=nothing)
 
 Set the parameter of a component `comp_name` in a model `m` to a given `value`.
-The `value` can by a scalar, an array, or a NamedAray. Optional argument 'dims'
+The `value` can by a scalar, an array, or a NamedAray. Optional keyword argument 'dims'
 is a list of the dimension names of the provided data, and will be used to check
 that they match the model's index labels.
 """
-@delegate set_param!(m::Model, comp_name::Symbol, param_name::Symbol, value, dims=nothing) => md
+@delegate set_param!(m::Model, comp_name::Symbol, param_name::Symbol, value; dims=nothing) => md
+
+"""
+    set_param!(m::Model, comp_name::Symbol, param_name::Symbol, ext_param_name::Symbol, value; dims=nothing)
+
+Set the parameter `param_name` of a component `comp_name` in a model `m` to a given `value`, 
+storing the value in the model's external parameter list by the provided name `ext_param_name`.
+The `value` can by a scalar, an array, or a NamedAray. Optional keyword argument 'dims'
+is a list of the dimension names of the provided data, and will be used to check
+that they match the model's index labels.
+"""
+@delegate set_param!(m::Model, comp_name::Symbol, param_name::Symbol, ext_param_name::Symbol, value; dims=nothing) => md
 
 
 """
-    set_param!(m::Model, param_name::Symbol, value, dims=nothing)
+    set_param!(m::Model, param_name::Symbol, value; dims=nothing)
 
 Set the value of a parameter in all components of the model that have a parameter of 
 the specified name.
 """
-@delegate set_param!(m::Model, param_name::Symbol, value, dims=nothing; ignoreunits::Bool=false) => md
+@delegate set_param!(m::Model, param_name::Symbol, value; dims=nothing, ignoreunits::Bool=false) => md
 
 @delegate import_params!(m::Model) => md
 
diff --git a/test/test_composite_parameters.jl b/test/test_composite_parameters.jl
index 25e9253b0..da2a9f1c2 100644
--- a/test/test_composite_parameters.jl
+++ b/test/test_composite_parameters.jl
@@ -1,3 +1,5 @@
+module TestCompositeParameters
+
 using Mimi
 using Test 
 
@@ -131,3 +133,5 @@ err8 = try set_param!(m2, :B, :p1, 2) catch err err end
 set_param!(m2, :B, :p1, :B_p1, 2)   # Use a unique name to set B.p1
 @test length(m2.md.external_param_conns) == 2 
 @test Set(keys(m2.md.external_params)) == Set([:p1, :B_p1])
+
+end
\ No newline at end of file
diff --git a/test/test_parameter_labels.jl b/test/test_parameter_labels.jl
index 9ffcad632..71b597e8e 100644
--- a/test/test_parameter_labels.jl
+++ b/test/test_parameter_labels.jl
@@ -265,7 +265,7 @@ model3 = Model()
 set_dimension!(model3, :time, collect(2015:5:2110))
 set_dimension!(model3, :regions, ["Region1", "Region2", "Region3"])
 add_comp!(model3, compA)
-set_param!(model3, :compA, :x, x, [:time, :regions])
+set_param!(model3, :compA, :x, x, dims = [:time, :regions])
 run(model3)
 
 for t in 1:length(time_labels)
diff --git a/test/test_parametertypes.jl b/test/test_parametertypes.jl
index 18ad8610a..bb0089145 100644
--- a/test/test_parametertypes.jl
+++ b/test/test_parametertypes.jl
@@ -259,4 +259,45 @@ update_params!(m, Dict(:x=>[3,4,5], :y=>[10,20], :z=>0), update_timesteps=true)
 @test external_param(m.md, :y).values == [10.,20.]
 @test external_param(m.md, :z).value == 0
 
+
+#------------------------------------------------------------------------------
+# Test the three different set_param! methods for a Symbol type parameter
+#------------------------------------------------------------------------------
+
+
+@defcomp A begin
+    p1 = Parameter{Symbol}()
+end
+
+function _get_model()
+    m = Model()
+    set_dimension!(m, :time, 10)
+    add_comp!(m, A)
+    return m
+end
+
+# Test the 3-argument version of set_param!
+m = _get_model()
+@test_throws MethodError set_param!(m, :p1, 3)  # Can't set it with an Int
+
+set_param!(m, :p1, :foo)    # Set it with a Symbol
+run(m)
+@test m[:A, :p1] == :foo
+
+# Test the 4-argument version of set_param!
+m = _get_model()
+@test_throws MethodError set_param!(m, :A, :p1, 3)
+
+set_param!(m, :A, :p1, :foo)
+run(m)
+@test m[:A, :p1] == :foo
+
+# Test the 5-argument version of set_param!
+m = _get_model()
+@test_throws MethodError set_param!(m, :A, :p1, :A_p1, 3)
+
+set_param!(m, :A, :p1, :A_p1, :foo)
+run(m)
+@test m[:A, :p1] == :foo
+
 end #module

From 053bd8685255b969bc808e53e263db0161e003fe Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Tue, 7 Apr 2020 15:04:06 -0400
Subject: [PATCH 14/25] revert to set_leftover_params! ignoring params with
 defaults

---
 src/core/connections.jl | 43 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/src/core/connections.jl b/src/core/connections.jl
index 644f33214..85bb21692 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -278,21 +278,6 @@ function split_datum_path(obj::AbstractCompositeComponentDef, s::AbstractString)
     return (ComponentPath(obj, elts[1]), Symbol(elts[2]))
 end
 
-# TBD: Deprecated?
-"""
-Connect a parameter and variable using string notation "/path/to/component:datum_name" where
-the potion before the ":" is the string representation of a component path from `obj` and the
-portion after is the name of the src or dst datum.
-"""
-function connect_param!(obj::AbstractCompositeComponentDef, dst::AbstractString, src::AbstractString,
-                        backup::Union{Nothing, Array}=nothing; ignoreunits::Bool=false, offset::Int=0)
-    dst_path, dst_name = split_datum_path(obj, dst)
-    src_path, src_name = split_datum_path(obj, src)
-
-    connect_param!(obj, dst_path, dst_name, src_path, src_name,
-                   backup; ignoreunits=ignoreunits, offset=offset)
-end
-
 """
     _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
 
@@ -455,23 +440,21 @@ function set_leftover_params!(md::ModelDef, parameters::Dict{T, Any}) where T
         comp_def = find_comp(md, comp_name)
         param_def = comp_def[param_name]
 
-
-        # @info "set_leftover_params: comp_name=$comp_name, param=$param_name"
-        # check whether we need to set the external parameter
-        _skip = false
-        if external_param(md, param_name, missing_ok=true) === nothing
-            if haskey(parameters, string(param_name))  
-                value = parameters[string(param_name)]
-                param_dims = parameter_dimensions(md, comp_name, param_name)
-
-                set_external_param!(md, param_name, value; param_dims = param_dims)
-            elseif param_def.default != nothing
-                _skip = true
-            else
-                error("Cannot set parameter :$param_name, not found in provided dictionary and no default value deteceted.")
+        # Only set the unconnected parameter if it doesn't have a default
+        if param_def.default === nothing
+            # check whether we need to create the external parameter
+            if external_param(md, param_name, missing_ok=true) === nothing
+                if haskey(parameters, string(param_name))  
+                    value = parameters[string(param_name)]
+                    param_dims = parameter_dimensions(md, comp_name, param_name)
+
+                    set_external_param!(md, param_name, value; param_dims = param_dims)
+                else
+                    error("Cannot set parameter :$param_name, not found in provided dictionary and no default value detected.")
+                end
             end
+            connect_param!(md, comp_name, param_name, param_name)
         end
-        _skip || connect_param!(md, comp_name, param_name, param_name)
     end
     nothing
 end

From 5c62b17f2d12a0df9e9909049728a89b67581d9e Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Wed, 8 Apr 2020 10:49:49 -0400
Subject: [PATCH 15/25] check for no connection errors before setting a param
 in the model's list

---
 src/core/defs.jl            | 11 +++++++++--
 test/test_getdataframe.jl   |  1 +
 test/test_parametertypes.jl | 21 +++++++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/core/defs.jl b/src/core/defs.jl
index 25994c048..dd74286d8 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -669,7 +669,13 @@ function set_param!(md::ModelDef, param_name::Symbol, value; dims=nothing, ignor
             values = value
         end
 
-        set_external_array_param!(md, ext_param_name, values, param_dims)
+        param = ArrayModelParameter(values, param_dims)
+        # Need to check the dimensions of the parameter data against each component before addeding it to the model's external parameters
+        for comp in comps
+            _check_labels(md, comp, param_name, param)
+        end
+        set_external_param!(md, ext_param_name, param)
+
 
     else # scalar parameter case
         value = convert(dtype, value)
@@ -678,7 +684,8 @@ function set_param!(md::ModelDef, param_name::Symbol, value; dims=nothing, ignor
 
     # connect_param! calls dirty! so we don't have to
     for comp in comps
-        connect_param!(md, comp, param_name, ext_param_name)
+        # Set check_labels=false because we already checked above before setting the param
+        connect_param!(md, comp, param_name, ext_param_name, check_labels=false)
     end
     nothing
 end
diff --git a/test/test_getdataframe.jl b/test/test_getdataframe.jl
index a1508606c..f0c020aed 100644
--- a/test/test_getdataframe.jl
+++ b/test/test_getdataframe.jl
@@ -43,6 +43,7 @@ set_param!(model1, :testcomp1, :par_scalar, 5.)
 add_comp!(model1, testcomp2)
 
 @test_throws ErrorException set_param!(model1, :testcomp2, :par2, late_first:5:early_last)
+@test ! (:par2 in keys(model1.md.external_params))  # Test that after the previous error, the :par2 didn't stay in the model's parameter list
 set_param!(model1, :testcomp2, :par2, years)
 
 # Test running before model built
diff --git a/test/test_parametertypes.jl b/test/test_parametertypes.jl
index bb0089145..862603327 100644
--- a/test/test_parametertypes.jl
+++ b/test/test_parametertypes.jl
@@ -300,4 +300,25 @@ set_param!(m, :A, :p1, :A_p1, :foo)
 run(m)
 @test m[:A, :p1] == :foo
 
+#------------------------------------------------------------------------------
+# Test that if set_param! errors in the connection step, 
+#       the created param doesn't remain in the model's list of params
+#------------------------------------------------------------------------------
+
+@defcomp A begin
+    p1 = Parameter(index = [time])
+end
+
+@defcomp B begin
+    p1 = Parameter(index = [time])
+end
+
+m = Model()
+set_dimension!(m, :time, 10)
+add_comp!(m, A)
+add_comp!(m, B)
+
+@test_throws ErrorException set_param!(m, :p1, 1:5)     # this will error because the provided data is the wrong size
+@test isempty(m.md.external_params)                     # But it should not be added to the model's dictionary
+
 end #module

From 9a5a4af02e735cee2b7e79bf11f89e7471fa4f41 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 16:28:55 -0400
Subject: [PATCH 16/25] move leaf ipc and epc collect functions into build file

---
 src/core/build.jl       | 94 +++++++++++++++++++++++++++++++++++++++++
 src/core/connections.jl | 94 -----------------------------------------
 2 files changed, 94 insertions(+), 94 deletions(-)

diff --git a/src/core/build.jl b/src/core/build.jl
index 2bbfe54e6..75e43ed46 100644
--- a/src/core/build.jl
+++ b/src/core/build.jl
@@ -92,6 +92,100 @@ function _instantiate_vars(md::ModelDef)
     return vdict
 end
 
+"""
+    _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
+
+Recurses through sub components and finds the full path(s) to desired datum, and their
+names at the leaf level. Returns a tuple (paths::Vector{ComponentPath}, datum_names::Vector{Symbol})
+"""
+function _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
+
+    # Base case-- leaf component
+    if obj isa ComponentDef
+        return ([nothing], [datum_name])
+    end
+
+    datumdef = obj[datum_name]
+    if datumdef isa CompositeVariableDef
+        refs = [datumdef.ref]   # CompositeVariableDef's can only point to one subcomponent
+    else
+        refs = datumdef.refs    # ComposteParameterDef's can have multiple refs
+    end
+
+    paths = []
+    datum_names = []
+
+    for ref in refs
+        # Get the comp and datum's for the current ref
+        next_obj = obj[ref.comp_name]
+        next_datum_name = ref.datum_name
+
+        # Recurse
+        sub_paths, sub_datum_names = _find_paths_and_names(next_obj, next_datum_name)
+
+        # Append the paths, and save with datum_names
+        for (sp, dn) in zip(sub_paths, sub_datum_names)
+            push!(paths, ComponentPath(next_obj.name, sp))
+            push!(datum_names, dn)
+        end
+    end
+
+    return (paths, datum_names)
+end
+
+"""
+    _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
+
+Returns a vector of InternalParameterConnections that represent all of the connections at the leaf level 
+that need to be made under the hood as specified by `conn`.
+"""
+function _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
+
+    top_dst_path = conn.dst_comp_path
+    comp = find_comp(md, top_dst_path)
+    comp !== nothing || error("Can't find $(top_dst_path) from $(md.comp_id)")
+    par_sub_paths, param_names = _find_paths_and_names(comp, conn.dst_par_name)
+    param_paths = [ComponentPath(top_dst_path, sub_path) for sub_path in par_sub_paths]
+
+    top_src_path = conn.src_comp_path
+    comp = find_comp(md, top_src_path)
+    comp !== nothing || error("Can't find $(top_src_path) from $(md.comp_id)")
+    var_sub_path, var_name = _find_paths_and_names(comp, conn.src_var_name)
+    var_path = ComponentPath(top_src_path, var_sub_path[1])
+
+    ipcs = [InternalParameterConnection(var_path, var_name[1], param_path, param_name, 
+        conn.ignoreunits, conn.backup; offset=conn.offset) for (param_path, param_name) in
+        zip(param_paths, param_names)]
+    return ipcs
+end
+
+
+"""
+    _get_leaf_level_epcs(md::AbstractCompositeComponentDef, epc::ExternalParameterConnection)
+
+Returns a vector that has a new ExternalParameterConnections that represent all of the connections at the leaf level 
+that need to be made under the hood as specified by `epc`.
+"""
+function _get_leaf_level_epcs(md::ModelDef, epc::ExternalParameterConnection)
+
+    comp = find_comp(md, epc.comp_path)
+    comp !== nothing || error("Can't find $(epc.comp_path) from $(md.comp_id)")
+    par_sub_paths, param_names = _find_paths_and_names(comp, epc.param_name)
+
+    leaf_epcs = ExternalParameterConnection[]
+    external_param_name = epc.external_param
+
+    top_path = epc.comp_path
+
+    for (par_sub_path, param_name) in zip(par_sub_paths, param_names)
+        param_path = ComponentPath(top_path, par_sub_path)
+        epc = ExternalParameterConnection(param_path, param_name, external_param_name)
+        push!(leaf_epcs, epc)
+    end
+
+    return leaf_epcs
+end
+
 # Collect all parameters with connections to allocated variable storage
 function _collect_params(md::ModelDef, var_dict::Dict{ComponentPath, Any})
 
diff --git a/src/core/connections.jl b/src/core/connections.jl
index 85bb21692..4a5071bf7 100644
--- a/src/core/connections.jl
+++ b/src/core/connections.jl
@@ -278,100 +278,6 @@ function split_datum_path(obj::AbstractCompositeComponentDef, s::AbstractString)
     return (ComponentPath(obj, elts[1]), Symbol(elts[2]))
 end
 
-"""
-    _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
-
-Recurses through sub components and finds the full path(s) to desired datum, and their
-names at the leaf level. Returns a tuple (paths::Vector{ComponentPath}, datum_names::Vector{Symbol})
-"""
-function _find_paths_and_names(obj::AbstractComponentDef, datum_name::Symbol)
-
-    # Base case-- leaf component
-    if obj isa ComponentDef
-        return ([nothing], [datum_name])
-    end
-
-    datumdef = obj[datum_name]
-    if datumdef isa CompositeVariableDef
-        refs = [datumdef.ref]   # CompositeVariableDef's can only point to one subcomponent
-    else
-        refs = datumdef.refs    # ComposteParameterDef's can have multiple refs
-    end
-
-    paths = []
-    datum_names = []
-
-    for ref in refs
-        # Get the comp and datum's for the current ref
-        next_obj = obj[ref.comp_name]
-        next_datum_name = ref.datum_name
-
-        # Recurse
-        sub_paths, sub_datum_names = _find_paths_and_names(next_obj, next_datum_name)
-
-        # Append the paths, and save with datum_names
-        for (sp, dn) in zip(sub_paths, sub_datum_names)
-            push!(paths, ComponentPath(next_obj.name, sp))
-            push!(datum_names, dn)
-        end
-    end
-
-    return (paths, datum_names)
-end
-
-"""
-    _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
-
-Returns a vector of InternalParameterConnections that represent all of the connections at the leaf level 
-that need to be made under the hood as specified by `conn`.
-"""
-function _get_leaf_level_ipcs(md::ModelDef, conn::InternalParameterConnection)
-
-    top_dst_path = conn.dst_comp_path
-    comp = find_comp(md, top_dst_path)
-    comp !== nothing || error("Can't find $(top_dst_path) from $(md.comp_id)")
-    par_sub_paths, param_names = _find_paths_and_names(comp, conn.dst_par_name)
-    param_paths = [ComponentPath(top_dst_path, sub_path) for sub_path in par_sub_paths]
-
-    top_src_path = conn.src_comp_path
-    comp = find_comp(md, top_src_path)
-    comp !== nothing || error("Can't find $(top_src_path) from $(md.comp_id)")
-    var_sub_path, var_name = _find_paths_and_names(comp, conn.src_var_name)
-    var_path = ComponentPath(top_src_path, var_sub_path[1])
-
-    ipcs = [InternalParameterConnection(var_path, var_name[1], param_path, param_name, 
-        conn.ignoreunits, conn.backup; offset=conn.offset) for (param_path, param_name) in
-        zip(param_paths, param_names)]
-    return ipcs
-end
-
-
-"""
-    _get_leaf_level_epcs(md::AbstractCompositeComponentDef, epc::ExternalParameterConnection)
-
-Returns a vector that has a new ExternalParameterConnections that represent all of the connections at the leaf level 
-that need to be made under the hood as specified by `epc`.
-"""
-function _get_leaf_level_epcs(md::ModelDef, epc::ExternalParameterConnection)
-
-    comp = find_comp(md, epc.comp_path)
-    comp !== nothing || error("Can't find $(epc.comp_path) from $(md.comp_id)")
-    par_sub_paths, param_names = _find_paths_and_names(comp, epc.param_name)
-
-    leaf_epcs = ExternalParameterConnection[]
-    external_param_name = epc.external_param
-
-    top_path = epc.comp_path
-
-    for (par_sub_path, param_name) in zip(par_sub_paths, param_names)
-        param_path = ComponentPath(top_path, par_sub_path)
-        epc = ExternalParameterConnection(param_path, param_name, external_param_name)
-        push!(leaf_epcs, epc)
-    end
-
-    return leaf_epcs
-end
-
 """
     connection_refs(obj::AbstractCompositeComponentDef)
 

From 847ec84fd0cbaafb5444a2f9375804dedbcb7772 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 16:56:39 -0400
Subject: [PATCH 17/25] clean up @defcomposite

---
 src/core/defcomposite.jl | 30 ++++++++----------------------
 src/core/defs.jl         |  2 +-
 2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index 8d1feccf5..7bd04bb34 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -78,7 +78,6 @@ end
 # Convert @defcomposite "shorthand" statements into Mimi API calls
 #
 function _parse(expr)
-    # valid_keys = (:default, :description, :visability, :unit)
     valid_keys = (:default, :description, :unit)
     result = nothing
 
@@ -188,34 +187,19 @@ macro defcomposite(cc_name, ex)
 end
 
 """
-    import_params!(obj::AbstractCompositeComponentDef;
-                   names::Union{Nothing,Vector{Symbol}}=nothing)
+    import_params!(obj::AbstractCompositeComponentDef)
 
 Imports all unconnected parameters below the given composite `obj` by adding references
 to these parameters in `obj`.
 
-NOT IMPLEMENTED: If `names` is not `nothing`, only the given names ar imported.
-
-This is called automatically by `build!()`, but it can be useful for developers of
-composites as well.
-
 N.B. This is also called at the end of code emitted by @defcomposite.
 """
-function import_params!(obj::AbstractCompositeComponentDef;
-                        names::Union{Nothing,Vector{Symbol}}=nothing)
+function import_params!(obj::AbstractCompositeComponentDef)
 
     unconn = unconnected_params(obj)
 
-    # verify that all explicit names are importable
-    if names !== nothing
-        error("CK: need to implement/verify what this behavior is")
-        unconn_names = [nameof(param_ref) for param_ref in unconn]
-        unknown = setdiff(names, unconn_names)
-        if ! isempty(unknown)
-            @error "Can't import names $unknown as these are not unconnected params"
-        end
-    end
-
+    # Check for unresolved parameter name collisions. 
+    # Users must explicitly define any parameters that come from multiple subcomponents.
     all_names = [ref.datum_name for ref in unconn]
     unique_names = unique(all_names)
     _map = Dict([name => count(isequal(name), all_names) for name in unique_names])
@@ -229,7 +213,8 @@ function import_params!(obj::AbstractCompositeComponentDef;
     end
 end
 
-function _find_collisions(fields, pairs)
+# Helper function for finding any field collisions for parameters that want to be joined
+function _find_collisions(fields, pairs::Vector{Pair{T, Symbol}}) where T
     collisions = Symbol[]
 
     pardefs = [comp.namespace[param_name] for (comp, param_name) in pairs]
@@ -271,6 +256,7 @@ function _resolve_composite_parameter_kwargs(obj::AbstractCompositeComponentDef,
     return new_kwargs
 end
 
+# Helper function for detecting whether a specified datum has already been imported or connected
 function _is_connected(obj::AbstractCompositeComponentDef, comp_name::Symbol, datum_name::Symbol)
     for (k, item) in obj.namespace
         if isa(item, AbstractCompositeParameterDef)
@@ -292,12 +278,12 @@ function _is_connected(obj::AbstractCompositeComponentDef, comp_name::Symbol, da
     # return UnnamedReference(comp_name, datum_name) in unconnected_params(obj)
 end
 
+# This function creates a CompositeParameterDef in the CompositeComponentDef obj
 function import_param!(obj::AbstractCompositeComponentDef, localname::Symbol,
                         pairs::Pair...; kwargs...)
 
     print_pairs = [(comp.comp_id, name) for (comp, name) in pairs]
     # @info "import_param!($(obj.comp_id), :$localname, $print_pairs)"
-    # @info "kwargs: $kwargs"
 
     for (comp, pname) in pairs
 
diff --git a/src/core/defs.jl b/src/core/defs.jl
index dd74286d8..76190f17f 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -598,7 +598,7 @@ function set_param!(md::ModelDef, param_name::Symbol, value; dims=nothing, ignor
     end
 
     # which fields to check for collisions in subcomponents
-    fields = ignoreunits ? [:dim_names, :datatype] : [:dim_names, :datatype, :unit]
+    fields = ignoreunits ? (:dim_names, :datatype) : (:dim_names, :datatype, :unit)
     collisions = _find_collisions(fields, [comp => param_name for comp in comps])
     if ! isempty(collisions) 
         if :unit in collisions

From d6e9e902e7fed5bd9d6cf4712c3c71d6e347da8d Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 16:57:10 -0400
Subject: [PATCH 18/25] remove old code from defs

---
 src/core/defs.jl | 81 ------------------------------------------------
 1 file changed, 81 deletions(-)

diff --git a/src/core/defs.jl b/src/core/defs.jl
index 76190f17f..f806557db 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -175,27 +175,6 @@ function _save_to_namespace(comp::AbstractComponentDef, key::Symbol, value::Name
     comp.namespace[key] = value
 end
 
-# """
-#     datum_reference(comp::ComponentDef, datum_name::Symbol)
-
-# Create a reference to the given datum, which must already exist.
-# """
-# function datum_reference(comp::ComponentDef, datum_name::Symbol)
-#     obj = _ns_get(comp, datum_name, AbstractDatumDef)
-#     path = @or(obj.comp_path, ComponentPath(comp.name))
-#     ref_type = obj isa ParameterDef ? ParameterDefReference : VariableDefReference
-#     return ref_type(datum_name, get_root(comp), path)
-# end
-
-# """
-#     datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
-
-# Create a reference to the given datum, which itself must be a DatumReference.
-# """
-# function datum_reference(comp::AbstractCompositeComponentDef, datum_name::Symbol)
-#     _ns_get(comp, datum_name, AbstractDatumReference)
-# end
-
 function Base.setindex!(comp::AbstractCompositeComponentDef, value::CompositeNamespaceElement, key::Symbol)
     _save_to_namespace(comp, key, value)
 end
@@ -346,11 +325,6 @@ parameter_names(md::ModelDef, comp_name::Symbol) = parameter_names(compdef(md, c
 
 parameter_names(comp_def::AbstractComponentDef) = collect(keys(param_dict(comp_def)))
 
-# parameter(obj::ComponentDef, name::Symbol) = _ns_get(obj, name, ParameterDef)
-
-# parameter(obj::AbstractCompositeComponentDef,
-#           name::Symbol) = dereference(_ns_get(obj, name, ParameterDefReference))
-
 parameter(obj::AbstractComponentDef, name::Symbol) = _ns_get(obj, name, AbstractParameterDef)
 
 parameter(obj::AbstractCompositeComponentDef, comp_name::Symbol,
@@ -358,11 +332,6 @@ parameter(obj::AbstractCompositeComponentDef, comp_name::Symbol,
 
 parameter(dr::ParameterDefReference) = parameter(compdef(dr), nameof(dr))
 
-# has_parameter(comp_def::ComponentDef, name::Symbol) = _ns_has(comp_def, name, ParameterDef)
-
-# has_parameter(comp_def::AbstractCompositeComponentDef,
-#               name::Symbol) = _ns_has(comp_def, name, ParameterDefReference)
-
 has_parameter(comp_def::AbstractComponentDef, name::Symbol) = _ns_has(comp_def, name, AbstractParameterDef)
 has_parameter(md::ModelDef, name::Symbol) = haskey(md.external_params, name)
 
@@ -396,39 +365,6 @@ function parameter_dimensions(obj::AbstractComponentDef, comp_name::Symbol, para
     return parameter_dimensions(compdef(obj, comp_name), param_name)
 end
 
-#
-# TBD: handling for importing/joining a set of parameters into a composite
-#
-# """
-#     new_set_param!(m::ModelDef, param_name::Symbol, value)
-
-# Search all model components, find all unbound parameters named `param_name`, create
-# external parameter called `param_name`, set its value to `value`, and create
-# connections from the external unbound parameters named `param_name` to the new
-# external parameter. If a prior external parameter `param_name` is found, raise error.
-# """
-# function new_set_param!(md::ModelDef, param_name::Symbol, value)
-#     comps = comps_with_unbound_param(md, param_name)
-#     new_set_param!(md, comps, param_name, value)
-# end
-
-# function new_set_param!(md::ModelDef, comp_list::Vector{ComponentDef},
-#                         param_name::Symbol, value)
-#     pairs = [pathof(cd) => param_name for cd in comp_list]
-#     new_set_param!(md, pairs, param_name, value)
-# end
-
-# function new_set_param!(md::ModelDef, pairs::Vector{Pair{ComponentPath, Symbol}},
-#                         ext_param_name::Symbol, value)
-#     if length(pairs) > 0
-#         param = set_external_param!(md, ext_param_name, value)
-
-#         for (comp_path, param_name) in pairs
-#             connect_param!(md, comp_path, param_name, ext_param_name)
-#         end
-#     end
-#     return nothing
-# end
 
 """
 Find and return a vector of tuples containing references to a ComponentDef and
@@ -501,14 +437,6 @@ function recurse(obj::ComponentDef, f::Function, args...;
     nothing
 end
 
-# function leaf_params(obj::AbstractCompositeComponentDef)
-#     params = []
-#     recurse(obj, x->append!(params, parameters(x)); leaf_only=true)
-
-#     root = get_root(obj)
-#     return [ParameterDefReference(nameof(param), root, pathof(param)) for param in params]
-# end
-
 # return UnnamedReference's for all subcomponents' parameters
 function subcomp_params(obj::AbstractCompositeComponentDef)
     params = UnnamedReference[]
@@ -995,15 +923,6 @@ function add_comp!(obj::AbstractCompositeComponentDef,
     _add_anonymous_dims!(obj, comp_def)
     _insert_comp!(obj, comp_def, before=before, after=after)
 
-    # Set parameters to any specified defaults, but only for leaf components
-    # if is_leaf(comp_def)
-    #     for param in parameters(comp_def)
-    #         if param.default !== nothing
-    #             save_default!(obj, comp_def, param)
-    #         end
-    #     end
-    # end
-
     # Return the comp since it's a copy of what was passed in
     return comp_def
 end

From b1b1af1a1b3511fa59d74b979bd515a105bb0b8a Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 17:20:04 -0400
Subject: [PATCH 19/25] remove all remaining appearances of
 ParameterDefReference and VariableDefReference

---
 src/core/defs.jl       | 57 +++---------------------------------------
 src/core/paths.jl      | 23 +++--------------
 src/core/show.jl       |  9 -------
 src/core/types/defs.jl | 37 ++++-----------------------
 4 files changed, 13 insertions(+), 113 deletions(-)

diff --git a/src/core/defs.jl b/src/core/defs.jl
index f806557db..1a1e028ed 100644
--- a/src/core/defs.jl
+++ b/src/core/defs.jl
@@ -8,8 +8,6 @@ end
 
 compdef(cr::ComponentReference) = find_comp(cr)
 
-compdef(dr::AbstractDatumReference) = find_comp(dr.root, dr.comp_path)
-
 compdef(obj::AbstractCompositeComponentDef, path::ComponentPath) = find_comp(obj, path)
 
 compdef(obj::AbstractCompositeComponentDef, comp_name::Symbol) = components(obj)[comp_name]
@@ -52,11 +50,6 @@ end
 
 dirty!(md::ModelDef) = (md.dirty = true)
 
-compname(dr::AbstractDatumReference) = dr.comp_path.names[end]
-
-is_variable(dr::AbstractDatumReference)  = has_variable(find_comp(dr), nameof(dr))
-is_parameter(dr::AbstractDatumReference) = has_parameter(find_comp(dr), nameof(dr))
-
 number_type(md::ModelDef) = md.number_type
 
 function number_type(obj::AbstractCompositeComponentDef)
@@ -117,10 +110,10 @@ istype(T::DataType) = (pair -> pair.second isa T)
 components(obj::AbstractCompositeComponentDef) = filter(istype(AbstractComponentDef), obj.namespace)
 
 param_dict(obj::ComponentDef) = filter(istype(ParameterDef), obj.namespace)
-param_dict(obj::AbstractCompositeComponentDef) = filter(istype(ParameterDefReference), obj.namespace)
+param_dict(obj::AbstractCompositeComponentDef) = filter(istype(CompositeParameterDef), obj.namespace)
 
 var_dict(obj::ComponentDef) = filter(istype(VariableDef), obj.namespace)
-var_dict(obj::AbstractCompositeComponentDef) = filter(istype(VariableDefReference), obj.namespace)
+var_dict(obj::AbstractCompositeComponentDef) = filter(istype(CompositeVariableDef), obj.namespace)
 
 """
     parameters(comp_def::AbstractComponentDef)
@@ -138,7 +131,7 @@ Return an iterator of the variable definitions (or references) for `comp_def`.
 """
 variables(obj::ComponentDef)  = values(filter(istype(VariableDef), obj.namespace))
 
-variables(obj::AbstractCompositeComponentDef) = values(filter(istype(VariableDefReference), obj.namespace))
+variables(obj::AbstractCompositeComponentDef) = values(filter(istype(CompositeVariableDef), obj.namespace))
 
 variables(comp_id::ComponentId)  = variables(compdef(comp_id))
 
@@ -179,7 +172,7 @@ function Base.setindex!(comp::AbstractCompositeComponentDef, value::CompositeNam
     _save_to_namespace(comp, key, value)
 end
 
-# Leaf components store ParameterDefReference or VariableDefReference instances in the namespace
+# Leaf components store DatumDef instances in the namespace
 function Base.setindex!(comp::ComponentDef, value::LeafNamespaceElement, key::Symbol)
     _save_to_namespace(comp, key, value)
 end
@@ -330,8 +323,6 @@ parameter(obj::AbstractComponentDef, name::Symbol) = _ns_get(obj, name, Abstract
 parameter(obj::AbstractCompositeComponentDef, comp_name::Symbol,
           param_name::Symbol) = parameter(compdef(obj, comp_name), param_name)
 
-parameter(dr::ParameterDefReference) = parameter(compdef(dr), nameof(dr))
-
 has_parameter(comp_def::AbstractComponentDef, name::Symbol) = _ns_has(comp_def, name, AbstractParameterDef)
 has_parameter(md::ModelDef, name::Symbol) = haskey(md.external_params, name)
 
@@ -634,10 +625,6 @@ function variable(obj::AbstractCompositeComponentDef, comp_path::ComponentPath,
     return variable(comp_def, var_name)
 end
 
-variable(dr::VariableDefReference) = variable(compdef(dr), nameof(dr))
-
-
-
 has_variable(comp_def::ComponentDef, name::Symbol) = _ns_has(comp_def, name, VariableDef)
 
 has_variable(comp_def::AbstractCompositeComponentDef, name::Symbol) = _ns_has(comp_def, name, CompositeVariableDef)
@@ -662,10 +649,7 @@ function variable_unit(obj::AbstractComponentDef, name::Symbol)
     return unit(var)
 end
 
-# Smooth over difference between VariableDef and VariableDefReference
 unit(obj::AbstractDatumDef) = obj.unit
-unit(obj::VariableDefReference)  = variable(obj).unit
-unit(obj::ParameterDefReference) = parameter(obj).unit
 
 """
     variable_dimensions(obj::AbstractCompositeComponentDef, comp_path::ComponentPath, var_name::Symbol)
@@ -831,25 +815,6 @@ function time_contains(outer::Dimension, inner::Dimension)
     return outer_idx[1] <= inner_idx[1] && outer_idx[end] >= inner_idx[end]
 end
 
-function _find_var_par(parent::AbstractCompositeComponentDef, comp_def::AbstractComponentDef,
-                       comp_name::Symbol, datum_name::Symbol)
-    path = ComponentPath(parent.comp_path, comp_name)
-    root = get_root(parent)
-
-    root === nothing && error("Component $(parent.comp_id) does not have a root")
-
-    if has_variable(comp_def, datum_name)
-        return VariableDefReference(datum_name, root, path)
-    end
-
-    if has_parameter(comp_def, datum_name)
-        @info "Calling ParameterDefReference($datum_name, $(root.comp_id), $path)"
-        return ParameterDefReference(datum_name, root, path)
-    end
-
-    error("Component $(comp_def.comp_id) does not have a data item named $datum_name")
-end
-
 """
     propagate_time!(obj::AbstractComponentDef, t::Dimension)
 
@@ -866,20 +831,6 @@ function propagate_time!(obj::AbstractComponentDef, t::Dimension)
     end
 end
 
-# Save the default value for a parameter, which is applied, if needed, at build time.
-function save_default!(obj::AbstractCompositeComponentDef, comp::AbstractComponentDef,
-                       param::ParameterDef)
-    root = get_root(obj)
-    path = pathof(comp)
-    name = nameof(param)
-    save_default!(obj, ParameterDefReference(name, root, path, param.default))
-end
-
-function save_default!(obj::AbstractCompositeComponentDef, ref::ParameterDefReference)
-    push!(obj.defaults, ref)
-    nothing
-end
-
 """
     add_comp!(
         obj::AbstractCompositeComponentDef,
diff --git a/src/core/paths.jl b/src/core/paths.jl
index 0e14797cb..7a93b7962 100644
--- a/src/core/paths.jl
+++ b/src/core/paths.jl
@@ -24,9 +24,8 @@ Base.joinpath(p1::ComponentPath, other...) = joinpath(joinpath(p1, other[1]), ot
     _fix_comp_path!(child::AbstractComponentDef, parent::AbstractCompositeComponentDef)
 
 Set the ComponentPath of a child object to extend the path of its composite parent.
-For composites, also update the component paths for all connections, and for all
-DatumReferences in the namespace. For leaf components, also update the ComponentPath
-for ParameterDefs and VariableDefs.
+For composites, also update the component paths for all connections. For leaf components, 
+also update the ComponentPath for ParameterDefs and VariableDefs.
 """
 function _fix_comp_path!(child::AbstractComponentDef, parent::AbstractCompositeComponentDef)
     parent_path = pathof(parent)
@@ -60,17 +59,6 @@ function _fix_comp_path!(child::AbstractComponentDef, parent::AbstractCompositeC
                                                    offset=conn.offset)
         end
 
-        for (name, ref) in ns(child)
-            if ref isa AbstractDatumReference
-                T = typeof(ref)
-                # @info "parent_path: $parent_path ref.comp_path: $(ref.comp_path)"
-                ref_comp = find_comp(parent, pathof(ref))
-                child[name] = new_ref = T(ref.name, root, pathof(ref_comp))
-                # @info "new path: $(new_ref.comp_path)"
-            end
-        end
-
-
     else
         for datum in [variables(child)..., parameters(child)...]
             # @info "Resetting leaf IPC from $(datum.comp_path) to $child_path"
@@ -83,9 +71,8 @@ end
     fix_comp_paths!(md::AbstractModelDef)
 
 Recursively set the ComponentPaths in a tree below a ModelDef to the absolute path equivalent.
-This includes updating the component paths for all internal/external connections, and all
-DatumReferences in the namespace. For leaf components, we also update the ComponentPath for
-ParameterDefs and VariableDefs.
+This includes updating the component paths for all internal/external connections. For leaf components, 
+we also update the ComponentPath for ParameterDefs and VariableDefs.
 """
 function fix_comp_paths!(md::AbstractModelDef)
     for child in compdefs(md)
@@ -162,8 +149,6 @@ function find_comp(obj::AbstractCompositeComponentDef, pathstr::AbstractString)
     find_comp(obj, path)
 end
 
-find_comp(dr::AbstractDatumReference) = find_comp(dr.root, dr.comp_path)
-
 find_comp(cr::AbstractComponentReference) = find_comp(parent(cr), pathof(cr))
 
 """
diff --git a/src/core/show.jl b/src/core/show.jl
index c8667caab..d0fd44616 100644
--- a/src/core/show.jl
+++ b/src/core/show.jl
@@ -188,15 +188,6 @@ function show(io::IO, obj::AbstractComponentDef)
     end
 end
 
-function show(io::IO, obj::VariableDefReference)
-    print(io, "VariableDefReference(name=:$(obj.name) path=$(obj.comp_path))")
-end
-
-function show(io::IO, obj::ParameterDefReference)
-    default = printable(obj.default)
-    print(io, "ParameterDefReference(name=:$(obj.name) path=$(obj.comp_path) default=$default)")
-end
-
 function show(io::IO, obj::ModelInstance)
     # Don't print full type signature since it's shown in .variables and .parameters
     print(io, "ModelInstance")
diff --git a/src/core/types/defs.jl b/src/core/types/defs.jl
index 605ee8655..edc00eeb5 100644
--- a/src/core/types/defs.jl
+++ b/src/core/types/defs.jl
@@ -10,8 +10,7 @@ end
 """
     nameof(obj::NamedDef) = obj.name
 
-Return the name of `def`.  `NamedDef`s include `DatumDef`, `ComponentDef`,
-`CompositeComponentDef`, and `VariableDefReference` and `ParameterDefReference`.
+Return the name of `def`.  `NamedDef`s include `DatumDef`, `ComponentDef`, and `CompositeComponentDef`
 """
 Base.nameof(obj::AbstractNamedObj) = obj.name
 
@@ -103,11 +102,11 @@ struct SubComponent <: MimiStruct
     alias::Union{Nothing, Symbol}
 end
 
+# UnnamedReferences are stored in CompositeParameterDefs or CompositeVariableDefs
+# to point to subcomponents' parameters or variables.
 struct UnnamedReference
-    # root::AbstractComponentDef
-    # comp_path::ComponentPath
-    comp_name::Symbol
-    datum_name::Symbol
+    comp_name::Symbol   # name of the referenced subcomponent
+    datum_name::Symbol  # name of the parameter or variable in the subcomponent's namespace
 end
 
 @class CompositeParameterDef <: ParameterDef begin
@@ -148,32 +147,6 @@ function CompositeVariableDef(name::Symbol, comp_path::ComponentPath, subcomp::A
     return CompositeVariableDef(name, comp_path, vardef.datatype, vardef.dim_names, vardef.description, vardef.unit, UnnamedReference(comp_name, vname))
 end
 
-# Stores references to the name of a component variable or parameter
-# and the ComponentPath of the component in which it is defined
-@class DatumReference <: NamedObj begin
-    # name::Symbol is inherited from NamedObj
-    root::AbstractComponentDef
-    comp_path::ComponentPath
-end
-
-Base.pathof(dr::AbstractDatumReference) = dr.comp_path
-
-@class ParameterDefReference <: DatumReference begin
-    default::Any    # allows defaults set in composites
-end
-
-function ParameterDefReference(name::Symbol, root::AbstractComponentDef,
-                               comp_path::ComponentPath)
-    return ParameterDefReference(name, root, comp_path, nothing)
-end
-
-@class VariableDefReference  <: DatumReference
-
-function dereference(ref::AbstractDatumReference)
-    comp = find_comp(ref)
-    return comp[ref.name]
-end
-
 # Define which types can appear in the namespace dict for leaf and composite compdefs
 global const LeafNamespaceElement      = AbstractDatumDef
 global const CompositeDatumDef         = Union{AbstractCompositeParameterDef, AbstractCompositeVariableDef}

From 4b2b057bfd6d00c38a6c1ed9ff2af8fb691f473c Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 17:33:52 -0400
Subject: [PATCH 20/25] remove @defmodel from tests

---
 src/core/defmodel.jl               | 127 +++++++++++++++--------------
 test/test_main.jl                  |  12 +--
 test/test_main_variabletimestep.jl |  11 +--
 test/test_units.jl                 |   2 +-
 4 files changed, 68 insertions(+), 84 deletions(-)

diff --git a/src/core/defmodel.jl b/src/core/defmodel.jl
index 4e722f47e..87eebcf54 100644
--- a/src/core/defmodel.jl
+++ b/src/core/defmodel.jl
@@ -14,66 +14,69 @@ Define a Mimi model. The following types of expressions are supported:
 4. `index[name] = iterable-of-values`           # define values for an index
 """
 macro defmodel(model_name, ex)
-    @capture(ex, elements__)
-
-    # @__MODULE__ is evaluated in calling module when macro is interpreted
-    result = :(
-        let calling_module = @__MODULE__, comp_mod_name = nothing, comp_mod_obj = nothing
-            global $model_name = Model()
-        end
-    )
-
-    # helper function used in loop below
-    function addexpr(expr)
-        let_block = result.args[end].args
-        push!(let_block, expr)
-    end
-
-    for elt in elements
-        offset = 0
-
-        if @capture(elt, component(comp_mod_name_.comp_name_)         | component(comp_name_) |
-                         component(comp_mod_name_.comp_name_, alias_) | component(comp_name_, alias_))
-
-            # set local copy of comp_mod_name to the stated or default component module
-            expr = (comp_mod_name === nothing ? :(comp_mod_obj = calling_module) # nameof(calling_module))
-                                              # TBD: This may still not be right:
-                                              : :(comp_mod_obj = getfield(calling_module, $(QuoteNode(comp_mod_name)))))
-            addexpr(expr)
-
-            name = (alias === nothing ? comp_name : alias)
-            expr = :(add_comp!($model_name, Mimi.ComponentId(comp_mod_obj, $(QuoteNode(comp_name))), $(QuoteNode(name))))
-
-
-        # TBD: extend comp.var syntax to allow module name, e.g., FUND.economy.ygross
-        elseif (@capture(elt, src_comp_.src_name_[arg_] => dst_comp_.dst_name_) ||
-                @capture(elt, src_comp_.src_name_ => dst_comp_.dst_name_))
-            if (arg !== nothing && (! @capture(arg, t - offset_) || offset <= 0))
-                error("Subscripted connection source must have subscript [t - x] where x is an integer > 0")
-            end
-
-            expr = :(Mimi.connect_param!($model_name,
-                                         $(QuoteNode(dst_comp)), $(QuoteNode(dst_name)),
-                                         $(QuoteNode(src_comp)), $(QuoteNode(src_name)),
-                                         offset=$offset))
-
-        elseif @capture(elt, index[idx_name_] = rhs_)
-            expr = :(Mimi.set_dimension!($model_name, $(QuoteNode(idx_name)), $rhs))
-
-        elseif @capture(elt, lhs_ = rhs_) && @capture(lhs, comp_.name_)
-            (path, param_name) = parse_dotted_symbols(lhs)
-            expr = :(Mimi.set_param!($model_name, $path, $(QuoteNode(param_name)), $rhs))
-
-        else
-            # Pass through anything else to allow the user to define intermediate vars, etc.
-            @info "Passing through: $elt"
-            expr = elt
-        end
-
-        addexpr(expr)
-    end
-
-    # addexpr(:($model_name))     # return this or nothing?
-    addexpr(:(nothing))
-    return esc(result)
+
+    @warn("@defmodel is deprecated.")
+    
+    # @capture(ex, elements__)
+
+    # # @__MODULE__ is evaluated in calling module when macro is interpreted
+    # result = :(
+    #     let calling_module = @__MODULE__, comp_mod_name = nothing, comp_mod_obj = nothing
+    #         global $model_name = Model()
+    #     end
+    # )
+
+    # # helper function used in loop below
+    # function addexpr(expr)
+    #     let_block = result.args[end].args
+    #     push!(let_block, expr)
+    # end
+
+    # for elt in elements
+    #     offset = 0
+
+    #     if @capture(elt, component(comp_mod_name_.comp_name_)         | component(comp_name_) |
+    #                      component(comp_mod_name_.comp_name_, alias_) | component(comp_name_, alias_))
+
+    #         # set local copy of comp_mod_name to the stated or default component module
+    #         expr = (comp_mod_name === nothing ? :(comp_mod_obj = calling_module) # nameof(calling_module))
+    #                                           # TBD: This may still not be right:
+    #                                           : :(comp_mod_obj = getfield(calling_module, $(QuoteNode(comp_mod_name)))))
+    #         addexpr(expr)
+
+    #         name = (alias === nothing ? comp_name : alias)
+    #         expr = :(add_comp!($model_name, Mimi.ComponentId(comp_mod_obj, $(QuoteNode(comp_name))), $(QuoteNode(name))))
+
+
+    #     # TBD: extend comp.var syntax to allow module name, e.g., FUND.economy.ygross
+    #     elseif (@capture(elt, src_comp_.src_name_[arg_] => dst_comp_.dst_name_) ||
+    #             @capture(elt, src_comp_.src_name_ => dst_comp_.dst_name_))
+    #         if (arg !== nothing && (! @capture(arg, t - offset_) || offset <= 0))
+    #             error("Subscripted connection source must have subscript [t - x] where x is an integer > 0")
+    #         end
+
+    #         expr = :(Mimi.connect_param!($model_name,
+    #                                      $(QuoteNode(dst_comp)), $(QuoteNode(dst_name)),
+    #                                      $(QuoteNode(src_comp)), $(QuoteNode(src_name)),
+    #                                      offset=$offset))
+
+    #     elseif @capture(elt, index[idx_name_] = rhs_)
+    #         expr = :(Mimi.set_dimension!($model_name, $(QuoteNode(idx_name)), $rhs))
+
+    #     elseif @capture(elt, lhs_ = rhs_) && @capture(lhs, comp_.name_)
+    #         (path, param_name) = parse_dotted_symbols(lhs)
+    #         expr = :(Mimi.set_param!($model_name, $path, $(QuoteNode(param_name)), $rhs))
+
+    #     else
+    #         # Pass through anything else to allow the user to define intermediate vars, etc.
+    #         @info "Passing through: $elt"
+    #         expr = elt
+    #     end
+
+    #     addexpr(expr)
+    # end
+
+    # # addexpr(:($model_name))     # return this or nothing?
+    # addexpr(:(nothing))
+    # return esc(result)
 end
diff --git a/test/test_main.jl b/test/test_main.jl
index 6e1a0e9e1..69d38cbe3 100644
--- a/test/test_main.jl
+++ b/test/test_main.jl
@@ -4,7 +4,7 @@ using Test
 using Mimi
 
 import Mimi: 
-    reset_variables, @defmodel, 
+    reset_variables, @defmodel,
     variable, variable_names, external_param, build, 
     compdefs, dimension, compinstance
 
@@ -25,16 +25,6 @@ import Mimi:
     var5 = Variable(index=[index1, idx4])
 end
 
-
-# @defmodel x1 begin
-#     index[index1] = [:r1, :r2, :r3]
-#     index[time] = 2010:10:2030
-#     index[idx3] = 1:3
-#     index[idx4] = 1:4
-#     component(foo1)
-
-#     foo1.par1 = 5.0
-# end
 x1 = Model()
 set_dimension!(x1, :index1, [:r1, :r2, :r3])
 set_dimension!(x1, :time, 2010:10:2030)
diff --git a/test/test_main_variabletimestep.jl b/test/test_main_variabletimestep.jl
index 9fb3fcf16..31ab5bbc1 100644
--- a/test/test_main_variabletimestep.jl
+++ b/test/test_main_variabletimestep.jl
@@ -4,7 +4,7 @@ using Test
 using Mimi
 
 import Mimi:
-    reset_variables, @defmodel,
+    reset_variables,
     variable, variable_names, external_param, build,
     compdef, compdefs, dimension, compinstance
 
@@ -25,15 +25,6 @@ import Mimi:
     var5 = Variable(index=[index1, idx4])
 end
 
-# @defmodel x1 begin
-#     index[index1] = [:r1, :r2, :r3]
-#     index[time] = [2010, 2015, 2030]
-#     index[idx3] = 1:3
-#     index[idx4] = 1:4
-#     component(foo1)
-
-#     foo1.par1 = 5.0
-# end
 x1 = Model()
 set_dimension!(x1, :index1, [:r1, :r2, :r3])
 set_dimension!(x1, :time, [2010, 2015, 2030])
diff --git a/test/test_units.jl b/test/test_units.jl
index 25f7996b1..d8e200391 100644
--- a/test/test_units.jl
+++ b/test/test_units.jl
@@ -3,7 +3,7 @@ module TestUnits
 using Test
 using Mimi
 
-import Mimi: verify_units, connect_param!, ComponentReference, @defmodel
+import Mimi: verify_units, connect_param!, ComponentReference
 
 # Try directly using verify_units
 @test verify_units("kg", "kg")

From b0d2f473c1f2989205f739dbcba4fdf3107c39ae Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Thu, 9 Apr 2020 19:38:45 -0400
Subject: [PATCH 21/25] clean composite test files

---
 test/test_composite.jl         |  4 ++-
 test/test_composite_simple.jl  | 10 ++----
 test/test_composite_simple2.jl | 58 ----------------------------------
 3 files changed, 6 insertions(+), 66 deletions(-)
 delete mode 100644 test/test_composite_simple2.jl

diff --git a/test/test_composite.jl b/test/test_composite.jl
index 3133061e8..d1ffbdbab 100644
--- a/test/test_composite.jl
+++ b/test/test_composite.jl
@@ -4,7 +4,7 @@ using Test
 using Mimi
 
 import Mimi:
-    ComponentId, ComponentPath, DatumReference, ComponentDef, AbstractComponentDef,
+    ComponentId, ComponentPath, ComponentDef, AbstractComponentDef,
     CompositeComponentDef, ModelDef, build, time_labels, compdef, find_comp,
     import_params!
 
@@ -157,6 +157,8 @@ set_dimension!(m2, :time, 2005:2020)
 
     connect(Comp2.par_2_1, Comp1.var_1_1)
     connect(Comp2.par_2_2, Comp1.var_1_1)
+
+    foo = Parameter(Comp1.foo, Comp2.foo)
 end
 
 top2_ref = add_comp!(m2, top2, nameof(top2))
diff --git a/test/test_composite_simple.jl b/test/test_composite_simple.jl
index de1fa70c7..d1e1df23d 100644
--- a/test/test_composite_simple.jl
+++ b/test/test_composite_simple.jl
@@ -1,10 +1,10 @@
-#module TestCompositeSimple
+module TestCompositeSimple
 
 using Test
 using Mimi
 
 import Mimi:
-    ComponentId, ComponentPath, DatumReference, ComponentDef, AbstractComponentDef,
+    ComponentId, ComponentPath, ComponentDef, AbstractComponentDef,
     CompositeComponentDef, ModelDef, build, time_labels, compdef, find_comp,
     import_params!
 
@@ -51,14 +51,10 @@ md = m.md
 set_dimension!(m, :time, 2005:2020)
 
 add_comp!(m, Top)
-top = md[:Top]
 
 set_param!(m, :Top, :fooA1, 10)
-
-import_params!(m)
 set_param!(m, :par_1_1, 1:16)
 
-build(m)
 run(m)
 
-#end
+end
diff --git a/test/test_composite_simple2.jl b/test/test_composite_simple2.jl
deleted file mode 100644
index d6b23dee1..000000000
--- a/test/test_composite_simple2.jl
+++ /dev/null
@@ -1,58 +0,0 @@
-#module TestCompositeSimple
-
-using Test
-using Mimi
-
-import Mimi:
-    ComponentId, ComponentPath, DatumReference, ComponentDef, AbstractComponentDef,
-    CompositeComponentDef, ModelDef, build, time_labels, compdef, find_comp,
-    import_params!
-
-@defcomp Leaf begin
-    p1 = Parameter()
-    v1 = Variable()
-
-    function run_timestep(p, v, d, t)
-        v.v1 = p.p1
-    end
-end
-
-@defcomposite Intermediate begin
-    Component(Leaf)
-    v1 = Variable(Leaf.v1)
-end
-
-@defcomposite Top begin
-    Component(Intermediate)
-    v = Variable(Intermediate.v1)
-    p = Parameter(Intermediate.p1)
-    connect(Intermediate.p1, Intermediate.v1)
-end
-
-
-m = Model()
-md = m.md
-set_dimension!(m, :time, 2005:2020)
-
-add_comp!(m, Top)
-
-top = md[:Top]
-inter = top[:Intermediate]
-leaf = inter[:Leaf]
-
-#
-# Two ways to set a value in a subcomponent of the ModelDef:
-#
-use_import = false
-if use_import
-    # import into model and reference it there
-    import_params!(m)
-    set_param!(m, :p, 10)
-else
-    # or, reference :p in Top,
-    set_param!(m, :Top, :p, 10)
-end
-
-build(m)
-run(m)
-#end

From 1b56a7b0686904120d1d521f99f74fa715c9d4c0 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Fri, 10 Apr 2020 10:01:47 -0400
Subject: [PATCH 22/25] improve @defcomposite error messages

---
 src/core/defcomposite.jl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/core/defcomposite.jl b/src/core/defcomposite.jl
index 7bd04bb34..b269b2ebd 100644
--- a/src/core/defcomposite.jl
+++ b/src/core/defcomposite.jl
@@ -204,11 +204,11 @@ function import_params!(obj::AbstractCompositeComponentDef)
     unique_names = unique(all_names)
     _map = Dict([name => count(isequal(name), all_names) for name in unique_names])
     non_unique = [name for (name, val) in _map if val>1]
-    isempty(non_unique) || error("There are unresolved parameter name collisions from subcomponents for the following parameter names: $(non_unique...).")
+    isempty(non_unique) || error("Cannot build composite :$(obj.name). There are unresolved parameter name collisions from subcomponents for the following parameter names: $(join(non_unique, ", ")).")
 
     for param_ref in unconn
         name = param_ref.datum_name
-        haskey(obj, name) && error("Failed to auto-import parameter :$name from component :$(param_ref.comp_name), this name has already been defined in the composite component's namespace.")
+        haskey(obj, name) && error("Cannot build composite :$(obj.name). Failed to auto-import parameter :$name from component :$(param_ref.comp_name), this name has already been defined in the composite component's namespace.")
         obj[name] = CompositeParameterDef(obj, param_ref)
     end
 end

From 0be96356d0b5bdb40f13745476ab46e33ed7719c Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Fri, 10 Apr 2020 12:47:43 -0400
Subject: [PATCH 23/25] add updated internals documentation

---
 docs/src/internals/composite_components.md    | 183 ------------------
 docs/src/internals/structures_1_overview.md   |  52 +++++
 .../src/internals/structures_2_definitions.md | 137 +++++++++++++
 docs/src/internals/structures_3_instances.md  |  49 +++++
 4 files changed, 238 insertions(+), 183 deletions(-)
 delete mode 100644 docs/src/internals/composite_components.md
 create mode 100644 docs/src/internals/structures_1_overview.md
 create mode 100644 docs/src/internals/structures_2_definitions.md
 create mode 100644 docs/src/internals/structures_3_instances.md

diff --git a/docs/src/internals/composite_components.md b/docs/src/internals/composite_components.md
deleted file mode 100644
index e9a1ae545..000000000
--- a/docs/src/internals/composite_components.md
+++ /dev/null
@@ -1,183 +0,0 @@
-# Composite Components
-
-## Overview
-
-This document describes the core data structures used to implement in Mimi 1.0.
-
-Prior versions of Mimi supported only "flat" models, i.e., with one level of components. The current version supports mulitple layers of components, with some components being "final" or leaf components, and others being "composite" components which themselves contain other leaf or composite components. This approach allows for a cleaner organization of complex models, and allows the construction of building blocks that can be re-used in multiple models.
-
-To the degree possible, composite components are designed to operate the same as leaf components, though there are necessarily differences:
-
-1. Leaf components are defined using the macro `@defcomp`, while composites are defined using `@defcomposite`. Each macro supports syntax and semantics specific to the type of component. See below for more details on these macros.
-
-1. Leaf composites support user-defined `run_timestep()` functions, whereas composites have a built-in `run_timestep()` function that iterates over its subcomponents and calls their `run_timestep()` function. The `init()` function is handled analogously.
-
-### Classes.jl
-
-Most of the core data structures are defined using the `Classes.jl` package, which was developed for Mimi, but separated out as a generally useful julia package. The main features of `Classes` are:
-
-1. Classes can subclass other classes, thereby inheriting the same list of fields as a starting point, which can then be extended with further fields.
-
-1. A type hierarchy is defined automatically that allows classes and subclasses to be referenced with a single type. In short, if you define a class `Foo`, an abstract type called `AbstractFoo` is defined, along with the concrete class `Foo`. If you subclass `Foo` (say with the class `Bar`), then `AbstractBar` will be a subtype of `AbstractFoo`, allowing methods to be defined that operate on both the superclass and subclass. See the Classes.jl documentation for further details.
-
-For example, in Mimi, `ModelDef` is a subclass of `CompositeComponentDef`, which in turn is a subclass of `ComponentDef`. Thus, methods can be written with arguments typed `x::ComponentDef` to operate on leaf components only, or `x::AbstractCompositeComponentDef` to operate on composites and `ModelDef`, or as `x::AbstractComponentDef` to operate on all three concrete types.
-
-## Core types
-
-These are defined in `types/core.jl`.
-
-1. `MimiStruct` and `MimiClass`
-
-All structs and classes in Mimi are derived from these abstract types, which allows us to identify Mimi-defined items when writing `show()` methods.
-
-1. `ComponentId`
-
-    To identify components, `@defcomp` creates a variable with the name of
-    the component whose value is an instance of this type. The definition is:
-
-    ```julia
-    struct ComponentId <: MimiStruct
-        module_obj::Union{Nothing, Module}
-        comp_name::Symbol
-    end
-    ```
-
-1. `ComponentPath`
-
-    A `ComponentPath` identifies the path from one or more composites to any component, using an `NTuple` of symbols. Since component names are unique at the composite level, the sequence of names through a component hierarchy uniquely identifies a component in that hierarchy.
-
-    ```julia
-    struct ComponentPath <: MimiStruct
-        names::NTuple{N, Symbol} where N
-    end
-    ```
-
-## Model Definition
-
-Models are composed of two separate structures, which we refer to as the "definition" side and the "instance" or "instantiated" side. The definition side is operated on by the user via the `@defcomp` and `@defcomposite` macros, and the public API.
-
-The instantiated model can be thought of as a "compiled" version of the model definition, with its data structures oriented toward run-time efficiency. It is constructed by Mimi in the `build()` function, which is called by the `run()` function.
-
-The public API sets a flag whenever the user modifies the model definition, and the instance is rebuilt before it is run if the model definition has changed. Otherwise, the model instance is re-run.
-
-The model definition is constructed from the following elements.
-
-### Leaf components
-
-1. `DatumDef`
-
-    This is a superclass holding elements common to `VariableDef` and `ParameterDef`, including the `ComponentPath` to the component in which the datum is defined, the data type, and dimension definitions. `DatumDef` subclasses are stored only in leaf components.
-
-1. `VariableDef <: DatumDef`
-
-    This class adds no new fields. It exists to differentiate variables from parameters.
-
-1. `ParameterDef <: DatumDef`
-
-    This class adds only a "default value" field to the `DatumDef`. Note that functions defined to operate on the `AbstractDatumDef` type work for both variable and parameter definitions.
-
-1. `ComponentDef`
-
-Instances of `ComponentDef` are defined using `@defcomp`. Their internal `namespace`
-dictionary can hold both `VariableDef` and `ParameterDef` instances.
-
-### Composite components
-
-Composite components provide a single component-like interface to an arbitrarily complex set
-of components (both leaf and composite components).
-
-1. `DatumReference`
-
-    This abstract class serves as a superclass for `ParameterDefReference`, and
-    `VariableDefReference`.
-
-1. `ParameterDefReference`, and `VariableDefReference`
-
-    These are used in composite components to store references to `ParameterDef` and `VariableDef` instances defined in leaf components. (They are conceptually like symbolic links in a
-    file system.) Whereas a `VariableDef` or `ParameterDef` can appear in a leaf
-    component, references to these may appear in any number of composite components.
-
-    "Importing" a parameter or variable from a sub-component defines a reference to that
-    datum in a leaf component. Note that if a composite imports a datum from another
-    composite, a reference to the leaf datum is stored in each case. That is, we don't
-    store references to references.
-
-1. `CompositeComponentDef <: ComponentDef`
-
-    Instances of `CompositeComponentDef` are defined using `@defcomposite`. Their internal `namespace` dictionary can hold instances of `ComponentDef`, `CompositeComponentDef`, `VariableDefReference` and `ParameterDefReference`.
-    Composite components also record internal parameter connections.
-
-1. `ModelDef <: CompositeComponentDef`
-
-    A `ModelDef` is a top-level composite that also stores external parameters and a list
-    of external parameter connections.
-
-### Parameter Connections
-
-Parameters hold values defined exogneously to the model ("external" parameters) or to the
-component ("internal" parameters).
-
-1. `InternalParameterConnection`
-
-Internal parameters are defined by connecting a parameter in one component to a variable
-in another component. This struct holds the names and `ComponentPath`s of the parameter
-and variable, and other information such as the "backup" data source. At build time,
-internal parameter connections result in direct references from the parameter to the
-storage allocated for the variable.
-
-1. `ExternalParameterConnection`
-
-Values that are exogenous to the model are defined in external parameters whose values are
-assigned using the public API function `set_param!()`, or by setting default values in
-`@defcomp` or `@defcomposite`, in which case, the default values are assigned via an
-internal call to `set_param!()`.
-
-External connections are stored in the `ModelDef`, along with the actual `ModelParameter`s,
-which may be scalar values or arrays, as described below.
-
-1. `ModelParameter`
-
-This is an abstract type that is the supertype of both `ScalarModelParameter{T}` and
-`ArrayModelParameter{T}`. These two parameterized types are used to store values set
-for external model parameters.
-
-## Instantiated Models
-
-
-
-1. `ComponentInstanceData`
-
-This is the supertype for variables and parameters in component instances.
-
-```julia
-@class ComponentInstanceData{NT <: NamedTuple} <: MimiClass begin
-    nt::NT
-    comp_paths::Vector{ComponentPath}   # records the origin of each datum
-end
-```
-
-1. `ComponentInstanceParameters`
-
-1. `ComponentInstanceVariables`
-
-1. `ComponentInstance`
-
-1. `LeafComponentInstance <: ComponentInstance`
-
-1. `CompositeComponentInstance <: ComponentInstance`
-
-    The `run_timestep()` method of a `ComponentInstance` simply calls the `run_timestep()`
-    method of each of its sub-components in dependency order.
-
-1. `ModelInstance <: CompositeComponentInstance`
-
-
-## User-facing Classes
-
-1. `Model`
-
-The `Model` class contains the `ModelDef`, and after the `build()` function is called, a `ModelInstance` that can be run. The API for `Model` delegates many calls to either its top-level `ModeDef` or `ModelInstance`, while providing additional functionality including running a Monte Carlo simulation.
-
-1. `ComponentReference`
-
-1. `VariableReference`
diff --git a/docs/src/internals/structures_1_overview.md b/docs/src/internals/structures_1_overview.md
new file mode 100644
index 000000000..f493a0846
--- /dev/null
+++ b/docs/src/internals/structures_1_overview.md
@@ -0,0 +1,52 @@
+
+# Overview
+
+This document (along with "structures_2_definitions" and "structures_3_instances") describes the core data structures used to implement in Mimi 1.0.
+
+Prior versions of Mimi supported only "flat" models, i.e., with one level of components. The current version supports mulitple layers of components, with some components being "final" or leaf components, and others being "composite" components which themselves contain other leaf or composite components. This approach allows for a cleaner organization of complex models, and allows the construction of building blocks that can be re-used in multiple models.
+
+To the degree possible, composite components are designed to operate the same as leaf components, though there are necessarily differences:
+
+1. Leaf components are defined using the macro `@defcomp`, while composites are defined using `@defcomposite`. Each macro supports syntax and semantics specific to the type of component. See below for more details on these macros.
+
+1. Leaf composites support user-defined `run_timestep()` functions, whereas composites have a built-in `run_timestep()` function that iterates over its subcomponents and calls their `run_timestep()` function. The `init()` function is handled analogously.
+
+## Classes.jl
+
+Most of the core data structures are defined using the `Classes.jl` package, which was developed for Mimi, but separated out as a generally useful julia package. The main features of `Classes` are:
+
+1. Classes can subclass other classes, thereby inheriting the same list of fields as a starting point, which can then be extended with further fields.
+
+1. A type hierarchy is defined automatically that allows classes and subclasses to be referenced with a single type. In short, if you define a class `Foo`, an abstract type called `AbstractFoo` is defined, along with the concrete class `Foo`. If you subclass `Foo` (say with the class `Bar`), then `AbstractBar` will be a subtype of `AbstractFoo`, allowing methods to be defined that operate on both the superclass and subclass. See the Classes.jl documentation for further details.
+
+For example, in Mimi, `ModelDef` is a subclass of `CompositeComponentDef`, which in turn is a subclass of `ComponentDef`. Thus, methods can be written with arguments typed `x::ComponentDef` to operate on leaf components only, or `x::AbstractCompositeComponentDef` to operate on composites and `ModelDef`, or as `x::AbstractComponentDef` to operate on all three concrete types.
+
+## Core types
+
+These are defined in `types/core.jl`.
+
+1. `MimiStruct` and `MimiClass`
+
+All structs and classes in Mimi are derived from these abstract types, which allows us to identify Mimi-defined items when writing `show()` methods.
+
+1. `ComponentId`
+
+    To identify components, `@defcomp` creates a variable with the name of
+    the component whose value is an instance of this type. The definition is:
+
+    ```julia
+    struct ComponentId <: MimiStruct
+        module_obj::Union{Nothing, Module}
+        comp_name::Symbol
+    end
+    ```
+
+1. `ComponentPath`
+
+    A `ComponentPath` identifies the path from one or more composites to any component, using an `NTuple` of symbols. Since component names are unique at the composite level, the sequence of names through a component hierarchy uniquely identifies a component in that hierarchy.
+
+    ```julia
+    struct ComponentPath <: MimiStruct
+        names::NTuple{N, Symbol} where N
+    end
+    ```
\ No newline at end of file
diff --git a/docs/src/internals/structures_2_definitions.md b/docs/src/internals/structures_2_definitions.md
new file mode 100644
index 000000000..e9bd38b0f
--- /dev/null
+++ b/docs/src/internals/structures_2_definitions.md
@@ -0,0 +1,137 @@
+# Definitions
+
+## Model Definition
+
+Models are composed of two separate structures, which we refer to as the "definition" side and the "instance" or "instantiated" side. The definition side is operated on by the user via the `@defcomp` and `@defcomposite` macros, and the public API (`add_comp!`, `set_param!`, `connect_param!`, etc.).
+
+The instantiated model can be thought of as a "compiled" version of the model definition, with its data structures oriented toward run-time efficiency. It is constructed by Mimi in the `build()` function, which is called by the `run()` function.
+
+The public API sets a flag whenever the user modifies the model definition, and the instance is rebuilt before it is run if the model definition has changed. Otherwise, the model instance is re-run.
+
+The model definition is constructed from the following elements.
+
+## Leaf components
+
+Leaf components are defined using the `@defcomp` macro which generates a component definition of the type `ComponentDef` which has the following fields:
+```
+# ComponentDef
+parent::Any
+name::Symbol
+comp_id::Union{Nothing, ComponentId}   
+comp_path::Union{Nothing, ComponentPath}
+dim_dict::OrderedDict{Symbol, Union{Nothing, Dimension}}
+namespace::OrderedDict{Symbol, Any}       
+first::Union{Nothing, Int}
+last::Union{Nothing, Int}
+is_uniform::Bool
+```
+The namespace of a leaf component can hold `ParameterDef`s and `VariableDef`s, both which are subclasses of `DatumDef` (see below for more details on these types).
+
+## Composite components
+
+Composite components are defined using the `@defcomposite` macro which generates a composite component definition of the type `CompositeComponentDef` which has the following fields, in addition to the fields of a `ComponentDef`:
+```
+# CompositeComponentDef <: ComponentDef 
+internal_param_conns::Vector{InternalParameterConnection}   
+backups::Vector{Symbol}
+sorted_comps::Union{Nothing, Vector{Symbol}}
+```
+The namespace of a composite component can hold `CompositeParameterDef`s and`CompositeVariableDef`s, as well as `AbstractComponentDef`s (which can be other leaf or composite component definitions).
+
+## Datum definitions
+
+Note: we use "datum" to refer collectively to parameters and variables. Parameters are values that are fed into a component, and variables are values calculated by a component's `run_timestep` function.
+
+Datum are defined with the `@defcomp` and `@defcomposite` macros, and have the following fields:
+```
+# DatumDef
+name::Symbol
+comp_path::Union{Nothing, ComponentPath}
+datatype::DataType
+dim_names::Vector{Symbol}
+description::String
+unit::String
+```
+The only difference between a ParameterDef and a VariableDef is that parameters can have default values.
+```
+# ParameterDef <: DatumDef
+default::Any
+
+# VariableDef <: DatumDef
+# (This class adds no new fields. It exists to differentiate variables from parameters.)
+```
+
+`CompositeParameterDef`s and `CompositeVariableDef`s are defined in the `@defcomposite` macro, and point to datum from their subcomponents. (Remember, composite components do not have `run_timestep` functions, so no values are actually calculated in a composite component.) Thus, `CompositeParameterDef`s and `CompositeVariableDef`s inherit all the fields from `ParameterDef`s and `VariableDef`s, and have an additional field to record which subcomponent(s)' datum they reference.
+```
+# CompositeParameterDef <: ParameterDef
+refs::Vector{UnnamedReference}
+
+# CompositeVariableDef <: VariableDef
+ref::UnnamedReference
+```
+Note: a `CompositeParameterDef` can reference multiple subcomponents' parameters, but a `CompositeVariableDef` can only reference a variable from one subcomponent.
+
+The reference(s) stored in `CompositeParameterDef`s and `CompositeVariableDef`s are of type `UnnamedReference`, which has the following fields:
+```
+# UnnamedReference
+comp_name::Symbol   # name of the referenced subcomponent
+datum_name::Symbol  # name of the parameter or variable in the subcomponent's namespace
+```
+
+## ModelDef
+
+A `ModelDef` is a top-level composite that also stores external parameters and a list of external parameter connections. It contains the following additional fields:
+```
+# ModelDef <: CompositeComponentDef
+external_param_conns::Vector{ExternalParameterConnection}
+external_params::Dict{Symbol, ModelParameter}
+number_type::DataType
+dirty::Bool
+```
+Note: a ModelDef's namespace will only hold `AbstractComponentDef`s. 
+
+## Parameter Connections
+
+Parameters hold values defined exogneously to the model ("external" parameters) or to the
+component ("internal" parameters).
+
+`InternalParameterConnection`
+Internal parameters are defined by connecting a parameter in one component to a variable
+in another component. This struct holds the names and `ComponentPath`s of the parameter
+and variable, and other information such as the "backup" data source. At build time,
+internal parameter connections result in direct references from the parameter to the
+storage allocated for the variable.
+
+`ExternalParameterConnection`
+Values that are exogenous to the model are defined in external parameters whose values are
+assigned using the public API function `set_param!()`, or by setting default values in
+`@defcomp` or `@defcomposite`, in which case, the default values are assigned via an
+internal call to `set_param!()`.
+
+External connections are stored in the `ModelDef`, along with the actual `ModelParameter`s,
+which may be scalar values or arrays, as described below.
+
+```
+# AbstractConnection
+
+# InternalParameterConnection <: AbstractConnection
+src_comp_path::ComponentPath      
+src_var_name::Symbol
+dst_comp_path::ComponentPath
+dst_par_name::Symbol
+ignoreunits::Bool
+backup::Union{Symbol, Nothing} # a Symbol identifying the external param providing backup data, or nothing
+offset::Int
+
+# ExternalParameterConnection  <: AbstractConnection
+comp_path::ComponentPath
+param_name::Symbol      # name of the parameter in the component
+external_param::Symbol  # name of the parameter stored in the model's external_params
+```
+
+## Model parameters 
+
+`ModelParameter`
+This is an abstract type that is the supertype of both `ScalarModelParameter{T}` and
+`ArrayModelParameter{T}`. These two parameterized types are used to store values set
+for external model parameters.
diff --git a/docs/src/internals/structures_3_instances.md b/docs/src/internals/structures_3_instances.md
new file mode 100644
index 000000000..61f7a132b
--- /dev/null
+++ b/docs/src/internals/structures_3_instances.md
@@ -0,0 +1,49 @@
+# Instances
+
+## Models and Components
+
+```
+# ComponentInstance
+comp_name::Symbol
+comp_id::ComponentID
+comp_path::ComponentPath (from top (model) down)
+first::Int
+last::Int
+
+# LeafComponentInstance <: ComponentInstance
+variables::ComponentInstanceVariables
+parameters::ComponentInstanceParameters
+init::Union{Nothing, Function}
+run_timestep::Union{Nothing, Function}
+
+# CompositeComponentInstance <: ComponentInstance
+comps_dict::OrderedDict{Symbol, ComponentInstance}
+pars_dict::OrderedDict{Symbol, Any}   # not currently being used but, probably should for better datum access
+vars_dict::OrderedDict{Symbol, Any}   # ^ same
+
+# ModelInstance <: CompositeComponentInstance
+md::ModelDef
+```
+
+## Datum
+
+```
+# ComponentInstanceParameters (only exist in leaf component instances)
+nt::NamedTuple{Tuple{Symbol}, Tuple{Type}}    # Type is either ScalarModelParameter (for scalar parameters) or TimestepArray (for array parameters)
+comp_paths::Vector{ComponentPath}
+
+# ComponentInstanceVariables (only exist in leaf component instances)
+nt::NamedTuple{Tuple{Symbol}, Tuple{Type}}  # Type is either ScalarModelParameter (for scalar variables) or TimestepArray (for array variables)
+comp_paths::Vector{ComponentPath}
+```
+Note: in the `ComponentInstanceParameters`, the values stored in the named tuple point to the actual variable arrays in the other components for things that are internally connected, or to the actual value stored in the mi.md.external_params dictionary if it's an external parameter. (So I'm not sure what the component paths are there for, because the component path seems to always reference the current component, even if the parameter data tehcnically originates from a different component.)
+
+## User-facing Classes
+
+1. `Model`
+
+The `Model` class contains the `ModelDef`, and after the `build()` function is called, a `ModelInstance` that can be run. The API for `Model` delegates many calls to either its top-level `ModeDef` or `ModelInstance`, while providing additional functionality including running a Monte Carlo simulation.
+
+1. `ComponentReference`
+
+1. `VariableReference`

From 09aed816edf2d85fe10c29e44b334a1220906528 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Fri, 10 Apr 2020 23:01:49 -0400
Subject: [PATCH 24/25] remove @defmodel from tests

---
 test/test_main.jl                      | 2 +-
 test/test_metainfo.jl                  | 9 ++++-----
 test/test_metainfo_variabletimestep.jl | 9 ++++-----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/test/test_main.jl b/test/test_main.jl
index 69d38cbe3..970d1c15e 100644
--- a/test/test_main.jl
+++ b/test/test_main.jl
@@ -4,7 +4,7 @@ using Test
 using Mimi
 
 import Mimi: 
-    reset_variables, @defmodel,
+    reset_variables,
     variable, variable_names, external_param, build, 
     compdefs, dimension, compinstance
 
diff --git a/test/test_metainfo.jl b/test/test_metainfo.jl
index 56521028f..7e311b51c 100644
--- a/test/test_metainfo.jl
+++ b/test/test_metainfo.jl
@@ -29,11 +29,10 @@ import Mimi:
     end
 end
 
-@Mimi.defmodel test_model begin
-    index[time] = 2010:2100
-    component(ch4forcing1)
-    component(ch4forcing1, ch4forcing2) # add another one with a different name
-end
+test_model = Model()
+set_dimension!(test_model, :time, 2010:2100)
+add_comp!(test_model, ch4forcing1)
+add_comp!(test_model, ch4forcing1, :ch4forcing2) # add another one with a different name
 
 c0 = ch4forcing1
 @test compmodule(c0) == TestMetaInfo
diff --git a/test/test_metainfo_variabletimestep.jl b/test/test_metainfo_variabletimestep.jl
index 8fe34fe42..3aa140abe 100644
--- a/test/test_metainfo_variabletimestep.jl
+++ b/test/test_metainfo_variabletimestep.jl
@@ -29,11 +29,10 @@ import Mimi:
     end
 end
 
-@Mimi.defmodel test_model begin
-    index[time] = [2010:2100; 2105:5:2200]
-    component(ch4forcing1)
-    component(ch4forcing1, ch4forcing2) # add another one with a different name
-end
+test_model = Model()
+set_dimension!(test_model, :time, [2010:2100; 2105:5:2200])
+add_comp!(test_model, ch4forcing1)
+add_comp!(test_model, ch4forcing1, :ch4forcing2) # add another one with a different name
 
 c1 = compdef(test_model, :ch4forcing1)
 c2 = compdef(test_model, :ch4forcing2)

From d7244a08117ef60b5a650d47fe5bce359ec4da12 Mon Sep 17 00:00:00 2001
From: Cora Kingdon <ckingdon@berkeley.edu>
Date: Sat, 11 Apr 2020 16:51:15 -0400
Subject: [PATCH 25/25] remove comment

---
 src/core/build.jl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/build.jl b/src/core/build.jl
index 75e43ed46..c6693f756 100644
--- a/src/core/build.jl
+++ b/src/core/build.jl
@@ -323,7 +323,7 @@ end
 function build(m::Model)
 
     # apply defaults to unset parameters
-    _set_defaults!(m.md)    # should this actually happen on the deepcopy below isntead?
+    _set_defaults!(m.md)
 
     # Reference a copy in the ModelInstance to avoid changes underfoot
     md = deepcopy(m.md)