Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ControlSignal costs are calculated twice #2721

Open
jvesely opened this issue Jul 7, 2023 · 7 comments
Open

ControlSignal costs are calculated twice #2721

jvesely opened this issue Jul 7, 2023 · 7 comments

Comments

@jvesely
Copy link
Collaborator

jvesely commented Jul 7, 2023

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
Copy link
Collaborator Author

jvesely commented Jul 7, 2023

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
Copy link
Collaborator Author

jvesely commented Jul 7, 2023

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

@jvesely
Copy link
Collaborator Author

jvesely commented Jul 7, 2023

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)

jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Jul 7, 2023
The function (TransferWithCosts) calculates costs during execution.

Closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Jul 7, 2023
The function (TransferWithCosts) calculates costs during execution.

Partly closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Jul 7, 2023
The function (TransferWithCosts) calculates costs during execution.
The expected results for the modulation simple test reflect halving of
calculated costs: [20, 24, 28, 32, 36] -> [10, 12, 14, 16, 18]

Partly closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
jvesely added a commit to jvesely/PsyNeuLink that referenced this issue Jul 7, 2023
The function (TransferWithCosts) calculates costs during execution.
The expected results for the modulation simple test reflect halving of
calculated costs: [20, 24, 28, 32, 36] -> [10, 12, 14, 16, 18]

Partly closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
@kmantel
Copy link
Collaborator

kmantel commented Jul 8, 2023

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 added a commit to jvesely/PsyNeuLink that referenced this issue Jul 10, 2023
The port function (TransferWithCosts) calculates costs during execution.

The expected results for the modulation simple test reflect halving of
calculated costs: [20, 24, 28, 32, 36] -> [10, 12, 14, 16, 18]

Partly closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
@jvesely
Copy link
Collaborator Author

jvesely commented Jul 10, 2023

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
Copy link
Collaborator

kmantel commented Jul 11, 2023

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 added a commit to jvesely/PsyNeuLink that referenced this issue Jul 11, 2023
The port function (TransferWithCosts) calculates costs during execution.

The expected results for the modulation simple test reflect halving of
calculated costs: [20, 24, 28, 32, 36] -> [10, 12, 14, 16, 18]

Partly closes: PrincetonUniversity#2721

Signed-off-by: Jan Vesely <[email protected]>
@jvesely
Copy link
Collaborator Author

jvesely commented Aug 4, 2023

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

removing the recalculation in _control_mechanism_costs_getter

@jvesely jvesely reopened this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants