ControlSignal costs are calculated twice

[jvesely]

the Port _update() method is overloaded in ControlSignal with the following:

    def _update(self, params=None, context=None):
        """Update value (intensity) and costs
        """
        super()._update(params=params, context=context)

        if self.parameters.cost_options._get(context):
            intensity = self.parameters.value._get(context)
            self.parameters.cost._set(self.compute_costs(intensity, context), context)

All ControlSignal costs are aliases for the function parameters of the TransferWithCosts function:

        function = Parameter(TransferWithCosts, stateful=False, loggable=False)
        [...]
        cost_options = FunctionParameter(
            CostFunctions.DEFAULTS,
            function_parameter_name=ENABLED_COST_FUNCTIONS,
            valid_types=(CostFunctions, list)
        )
        intensity_cost = FunctionParameter(None)
        adjustment_cost = FunctionParameter(0)
        duration_cost = FunctionParameter(0)

        cost = FunctionParameter(None, function_parameter_name=COMBINED_COSTS)

        intensity_cost_function = FunctionParameter(
            Exponential,
            function_parameter_name='intensity_cost_fct'
        )
        adjustment_cost_function = FunctionParameter(
            Linear,
            function_parameter_name='adjustment_cost_fct'
        )
        duration_cost_function = FunctionParameter(
            SimpleIntegrator,
            function_parameter_name='duration_cost_fct'
        )
        combine_costs_function = FunctionParameter(
            Reduce,
            function_parameter_name='combine_costs_fct'
        )

thus the above method first calls the TransferWithCosts function, which calculates all the enabled costs for the current intensity and then proceeds to call execute self.compute_costs with the current intensity again.

[jvesely]

Removing the def _update(self, params=None, context=None): overload fails only one test:

tests/composition/test_control.py::TestControlMechanisms::test_modulation_simple[ExecutionMode.Python-CostFunctions.DURATION--]
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0
E           
E           Mismatched elements: 5 / 5 (100%)
E           Max absolute difference: 18.
E           Max relative difference: 0.62068966
E            x: array([ -7.,  -8.,  -9., -10., -11.])
E            y: array([-17, -20, -23, -26, -29]

The test modulations are [1,2,3,4,5] which modulate intercept parameter of a TransferFunction. The input to the function is 2, so the net outcome is 2 + modulation - costs That gives original costs of [20, 24, 28, 32, 36] which are double the new (after removing _update) costs: [10, 12, 14, 16, 18]

[jvesely]

likely, both def _update and def calculate_costs can be removed since they just duplicate the calculations done in TransferWithCosts

[jvesely]

Note that this is not the only place that recalculates the costs. composition _get_total_cost_of_control_allocation, calls controller.parameters.costs_get which executes _control_mechanism_costs_getter which again recalculates all the costs that have just been calculated by applying allocation and updating the control signals. Hence even the above new costs ([10, 12, 14, 16, 18]) apply the modulation value twice (each cost is 2 * modulation + 8)

[kmantel]

Considering duration cost uses an integrator by default, I wonder if instead removing the recalculation in _control_mechanism_costs_getter is the way to go. It seems like it could easily end up in a situation where a few calls to the getter end up unintentionally pushing the integration further than expected, so maybe it would be better to have costs only get updated in a function called once during some execution interval. I'm not sure if that would be Port._update or elsewhere.

Side note - this is also related to #2695 I think.

[jvesely]

Considering duration cost uses an integrator by default, I wonder if instead removing the recalculation in _control_mechanism_costs_getter is the way to go. It seems like it could easily end up in a situation where a few calls to the getter end up unintentionally pushing the integration further than expected, so maybe it would be better to have costs only get updated in a function called once during some execution interval. I'm not sure if that would be Port._update or elsewhere.

Even if the getter is fixed the current overload of def _update would run the cost calculation twice on port update (see #2722), so I think we need both the removal of def _update and a fix to the getter for a complete solution.

Side note - this is also related to #2695 I think.

Could this be why the costs appear to start at 8 in the test_modulation_simple?

[kmantel]

Even if the getter is fixed the current overload of def _update would run the cost calculation twice on port update (see #2722), so I think we need both the removal of def _update and a fix to the getter for a complete solution.

Got it, I see that now. Makes sense.

Could this be why the costs appear to start at 8 in the test_modulation_simple?

Yes it could be. Right now the integration should be a couple steps too far after initialization, and after this issue is solved it should just be a single step ahead.

[jvesely]

I think we should leave this open until all the other issues are investigated e.g.:

removing the recalculation in _control_mechanism_costs_getter