fix: parameter binding logic

One of the concepts that made debugging the parameter bindings difficult is
that ultimately the parameter bindings refer to the vector of _free
parameters_, not the immediate parameters of the model. That is, the presence
of frozen parameters could throw off the expected binding resolution.

To fix this, we defer adjusting the parameter indices until we construct the
fit, and introduce a new function `adjust_free_bindings` which will do the
index shuffling and remove frozen parameter bindings.

The bindings that are kept in `prob.bindings` are now the expected model =>
parameter index mappings without the frozen adjustment, which makes reasoning
whether a binding is correct or not that bit easier.

src/fitting/binding.jl

@@ -50,12 +50,6 @@ function _get_index_of_symbol(model::AbstractSpectralModel, symbol)::Int
     if isnothing(i)
         error("Could not match symbol $symbol !")
     end
-    # don't count frozen parameters if they are prior to the parameter of interest
-    for s = 1:i
-        if isfrozen(pnt[s])
-            i -= 1
-        end
-    end
     i
 end
 

src/fitting/cache.jl

@@ -111,24 +111,73 @@ function _invoke_and_transform!(cache::MultiModelCache, domain, params)
     all_outputs
 end
 
+"""
+    adjust_free_bindings(model::FittableMultiModel, bindings::Vector{Vector{Pair{Int,Int}}})
+
+Returns a new parameter binding list with the parameter indices adjusted to
+omit the frozen parameter. That is, if a model has three parameters `a`, `b`
+(frozen) and `c`, and parameter `c` (index 3) is bound; then the new bindings
+will instead give the index of `c` as 2. 
+
+In that sense, the new bindings refer to the _free parameter_ vector, and not the full parameter vector. 
+
+If the binding refers to a frozen parameter, it is removed from the binding list.
+"""
+function adjust_free_bindings(model::FittableMultiModel, bindings)
+    new_bindings = map(bindings) do binding
+        pairs = Vector{Pair{Int,Int}}()
+        for pair in binding
+            model_index, parameter_index = pair
+
+            new_index = parameter_index
+            refers_to_frozen = false
+
+            for (i, param) in enumerate(parameter_tuple(model.m[model_index]))
+                if i > parameter_index
+                    break
+                end
+
+                if isfrozen(param)
+                    if i == parameter_index
+                        refers_to_frozen = true
+                        break
+                    end
+                    new_index -= 1
+                end
+            end
+
+            if !refers_to_frozen
+                push!(pairs, model_index => new_index)
+            end
+        end
+        pairs
+    end
+
+    # remove those that have become redundant
+    filter(i -> length(i) > 1, new_bindings)
+end
+
 function _build_parameter_mapping(model::FittableMultiModel{M}, bindings) where {M}
     T = paramtype(M.parameters[1])
 
-    all_parameters = Vector{T}()
+    all_free_parameters = Vector{T}()
     # use the tuple hack to enforce type stability and unroll the loop
     parameters = map((1:length(M.parameters)...,)) do i
         m = model.m[i]
         v::Vector{T} = collect(filter(isfree, parameter_tuple(m)))
-        append!(all_parameters, v)
+        append!(all_free_parameters, v)
         v
     end
     parameters_counts = _accumulated_indices(map(length, parameters))
 
-    parameter_mapping, remove = _construct_bound_mapping(bindings, parameters_counts)
+    # need to adjust the bindings to remove the frozen indices
+    free_bindings = adjust_free_bindings(model, bindings)
+
+    parameter_mapping, remove = _construct_bound_mapping(free_bindings, parameters_counts)
     # remove duplicate parameters that are bound
-    deleteat!(all_parameters, remove)
+    deleteat!(all_free_parameters, remove)
 
-    all_parameters, parameter_mapping
+    all_free_parameters, parameter_mapping
 end
 
 function _build_mapping_length(f, itt::Tuple)

test/fitting/test-binding.jl

@@ -85,6 +85,11 @@ prob = FittingProblem(model1 => dummy_data1, model1 => dummy_data1, model1 => du
 model1.K_1.frozen = true
 bind!(prob, :a_1)
 bind!(prob, :a_2)
+
+# check that the free parameter binding adjustment works okay
+new_bindings = SpectralFitting.adjust_free_bindings(prob.model, prob.bindings)
+@test new_bindings == [[1 => 1, 2 => 1, 3 => 1], [1 => 3, 2 => 3, 3 => 3]]
+
 _, mapping = SpectralFitting._build_parameter_mapping(prob.model, prob.bindings)
 @test mapping == ([1, 2, 3], [1, 4, 3], [1, 5, 3])
 
@@ -94,5 +99,27 @@ model1.K_1.frozen = false
 model1.a_2.frozen = true
 bind!(prob, :K_1)
 bind!(prob, :a_1)
+
+new_bindings = SpectralFitting.adjust_free_bindings(prob.model, prob.bindings)
+@test new_bindings == [[1 => 1, 2 => 1, 3 => 1], [1 => 2, 2 => 2, 3 => 2]]
+
 _, mapping = SpectralFitting._build_parameter_mapping(prob.model, prob.bindings)
 @test mapping == ([1, 2, 3], [1, 2, 4], [1, 2, 5])
+
+
+prob = FittingProblem(model1 => dummy_data1, model1 => dummy_data1, model1 => dummy_data1)
+
+# bind model 1's K_1 parameter to model 2's K_2 parameter
+bind!(prob, 1 => :K_1, 2 => :K_2)
+
+new_bindings = SpectralFitting.adjust_free_bindings(prob.model, prob.bindings)
+@test new_bindings == [[1 => 1, 2 => 3]]
+
+# bind model 1's a_1 parameter to model 2's a_2 to model 3's a_2
+# these are all frozen so should not appear in the adjust_free_bindings
+bind!(prob, 1 => :a_1, 2 => :a_2, 3 => :a_2)
+
+new_bindings = SpectralFitting.adjust_free_bindings(prob.model, prob.bindings)
+@test new_bindings == [[1 => 1, 2 => 3]]
+
+# TODO: free parameters should not be allowed to bind to frozen parameters