[RTR] Parameters uniformisation

[EveCharbie]

All Submissions:

• Have you followed the guidelines in our Contributing document [docs/contribution.md]?
• Have you checked to ensure there aren't other open [Pull Requests] for the same update/change?
• Have you opened/linked the issue related to your pull request? issue Parameter implementation should be uniformed with the other variables  #686
• Have you used the tag [WIP] for on-going changes, and removed it when the pull request was ready?
• When ready to merge, have you sent a comment pinging @pariterre in it?

New Feature Submissions:

1. Does your submission pass the tests (if not please explain why this is intended)?
2. Did you write a proper documentation (docstrings and ReadMe)
3. Have you linted your code locally prior to submission (using the command: black . -l120 --exclude "external/*")?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new examples for your core changes, as applicable?
• Have you written new tests for your core changes, as applicable?

---

This change is 

[EveCharbie]

@pariterre I am not ready for a full review, but we would need to make a decision regarding parameters.mx_reduced (line 1082 in configure_problem for example).

[pariterre]

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @EveCharbie)

---

bioptim/dynamics/configure_problem.py line 1082 at r1 (raw file):

            nlp.states["q"].mapping.to_second.map(q), nlp.states["qdot"].mapping.to_second.map(qdot)
        )
        # @pariterre: what do we do with this ? nlp.parameters.mx is not symbolic and mx_reduced is not the one passed to the model

This is precisely the most difficult part of which... If we want to be standard with the rest of the code, either we pass the mx_reduced to the model (meaning it is up to the user to convert it to mx) or we change states and controls to be passing the mx instead of mx_reduced to ipopt, which would render useless the scaling all together.

The first way is probably the best course of action, but that mean we should make sure the controller sent to the user does the proper job of converting what is needed and received

---

bioptim/examples/getting_started/custom_parameters.py line 236 at r1 (raw file):

    x_scaling = VariableScalingList()
    x_scaling.add("q", [3.14, 3.14])
    x_scaling.add("qdot", [10, 10])

Indeed

---

bioptim/limits/penalty_helpers.py line 127 at r1 (raw file):

    @staticmethod
    def parameters(penalty, get_parameter_decision: Callable):
        p = get_parameter_decision(None, None, None)

Make it pass the phase, node and subnode here (even though they will be ignored by the receiver of the callback, it will be useful when we implement the phase parameters!

---

bioptim/limits/penalty_option.py line 508 at r1 (raw file):

            else:
                u_end = controller.controls_scaled.cx_end
            p_start = controller.parameters_scaled.cx_start

.cx should be also callable and also return cx_start, which would be easier to understand (as cx_start is a bit weird in this context)

---

bioptim/limits/penalty_option.py line 522 at r1 (raw file):

            self.function[node] = Function(
                f"{name}",
                [time_cx, dt, x, u, p, a],

If we loose the "_cx" should we loose it for time_cx as well?

---

bioptim/limits/penalty_option.py line 534 at r1 (raw file):

            self.function[node] = Function(
                name,
                [time_cx, dt, x, u, p, a],  # replace all by dt ????

p is a bit dangerous as the phase_idx counter is often p... if it is not the case here, p is okay

---

bioptim/optimization/non_linear_program.py line 199 at r1 (raw file):

        from ..optimization.parameters import ParameterList

        self.parameters = ParameterList(use_sx=True if self.cx == SX else False)

change use_sx for cx_constructor (or something like that, see OptimizationVariable) and directly pass the self.cx.
If it is better to use use_sx then simply do use_sx=self.cx == SX (no need for True if True else False)

---

bioptim/optimization/optimal_control_program.py line 1567 at r1 (raw file):

            size=params_cx.shape[0],
            mapping=self.time_phase_mapping,
            scaling=VariableScaling("name", np.array([1])),

use the actual name of the param? f"{param_name}_scaled"

---

bioptim/optimization/optimization_vector.py line 134 at r1 (raw file):

        u_scaled = []
        a_scaled = []
        param_scaled = ocp.parameters.scaled.cx

If we keep p elsewhere, this can be changed for p_scaled I think

---

bioptim/optimization/parameters.py line 239 at r1 (raw file):

#     def add(
#         self,

To be removed

[EveCharbie]

Reviewable status: 10 of 13 files reviewed, 10 unresolved discussions (waiting on @pariterre)

---

bioptim/dynamics/configure_problem.py line 1082 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

This is precisely the most difficult part of which... If we want to be standard with the rest of the code, either we pass the mx_reduced to the model (meaning it is up to the user to convert it to mx) or we change states and controls to be passing the mx instead of mx_reduced to ipopt, which would render useless the scaling all together.

The first way is probably the best course of action, but that mean we should make sure the controller sent to the user does the proper job of converting what is needed and received

Think I fixed it otherwise :)

---

bioptim/limits/penalty_helpers.py line 127 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Make it pass the phase, node and subnode here (even though they will be ignored by the receiver of the callback, it will be useful when we implement the phase parameters!

Done.

---

bioptim/limits/penalty_option.py line 508 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

.cx should be also callable and also return cx_start, which would be easier to understand (as cx_start is a bit weird in this context)

Done.

---

bioptim/limits/penalty_option.py line 522 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

If we loose the "_cx" should we loose it for time_cx as well?

Done.

---

bioptim/limits/penalty_option.py line 534 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

p is a bit dangerous as the phase_idx counter is often p... if it is not the case here, p is okay

I think I removed all the ocurence of looping over 'p' as a phase index

---

bioptim/optimization/non_linear_program.py line 199 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

change use_sx for cx_constructor (or something like that, see OptimizationVariable) and directly pass the self.cx.
If it is better to use use_sx then simply do use_sx=self.cx == SX (no need for True if True else False)

I did not expect the user to know how to call MX vs SX, the user would have to do parameter_list.add(..., cx_constructer=casadi.MX())

---

bioptim/optimization/optimal_control_program.py line 1567 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

use the actual name of the param? f"{param_name}_scaled"

Done.

---

bioptim/optimization/optimization_vector.py line 134 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

If we keep p elsewhere, this can be changed for p_scaled I think

Done.

---

bioptim/optimization/parameters.py line 239 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

To be removed

Done.

[EveCharbie]

Reviewable status: 1 of 37 files reviewed, 10 unresolved discussions (waiting on @pariterre)

---

bioptim/examples/getting_started/custom_parameters.py line 236 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Indeed

Done.

[EveCharbie]

@pariterre I would need help for the remaining tests :)

[pariterre]

Reviewed 3 of 11 files at r3, 34 of 34 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @EveCharbie)

---

bioptim/dynamics/integrator.py line 65 at r4 (raw file):

        self.t_span_sym = ode["t"]
        self.x_sym = ode["x"]
        self.u_sym = ode["p"]  # TODO: @ pariterre could we put ode["u"] instead?

We could, but this was to be consistent with Ipopt nomenclature (as "controls" does not exist for Ipopt only parameters. That said, I always found it confusing

---

bioptim/interfaces/interface_utils.py line 244 at r4 (raw file):

        phases_dt = PenaltyHelpers.phases_dt(penalty, interface.ocp, lambda _: interface.ocp.dt_parameter.cx)
        _, _, _, p, _, _, _ = _get_weighted_function_inputs(penalty, 0, ocp, nlp, scaled)

Is there a reason here why PenaltyHelpers.parameters does not work or is it just to reduce the amount of duplicated code? If it is the latter, I think we could argue that instead of discarding the p some line later, we could just retrieve it and remove the current line

I meant here:
t0_tp, x_tp, u_tp, _, a_tp, weight_tp, target_tp = _get_weighted_function_inputs(
penalty, idx, ocp, nlp, scaled
)

---

bioptim/limits/penalty.py line 1430 at r4 (raw file):

            markers,
            controller.time,
            controller.parameters,

What if parameters changed the computation of qddot (changing the gravity for instance)?

---

bioptim/optimization/non_linear_program.py line 199 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

I did not expect the user to know how to call MX vs SX, the user would have to do parameter_list.add(..., cx_constructer=casadi.MX())

I see... Can this be done retroactively when calling OptimizationControlProgram? (I mean that the use_sx or cx_constructor are set by OCP or the add method needs to know already...)

EDIT: Ok, this is what you eventually did by using Container instead of List. :)

---

bioptim/optimization/non_linear_program.py line 450 at r1 (raw file):

                cx += [var.cx_start]
            else:
                cx += [var.cx]  # This is a temporary hack until parameters are included as OptimizationVariables

How long this "hack" lasted? hahahaha

---

bioptim/optimization/optimization_vector.py line 442 at r4 (raw file):

        # For parameters
        for i_param, key in enumerate(ocp.parameters):

the i_param does not seem to be used, did you mean ocp.parameters.keys()?

---

bioptim/optimization/parameters.py line 251 at r4 (raw file):

    def __init__(self):
        super(ParameterContainer, self).__init__(phase_dynamics=PhaseDynamics.SHARED_DURING_THE_PHASE)
        self._scaled: ParameterList = ParameterList(use_sx=True)

use_sx=True?

---

bioptim/optimization/variable_scaling.py line 27 at r4 (raw file):

            raise ValueError(
                f"Scaling must be a 1- or 2- dimensional numpy array"
            )  # Pariterre: in which case do we want 2D?

I think it refers to the fact that one-column matrix are 2d arrays (like what we are doing in prior with the np.newaxis. I do not think it means it should be a matrix with more than one column though

[EveCharbie]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pariterre)

---

bioptim/optimization/non_linear_program.py line 199 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I see... Can this be done retroactively when calling OptimizationControlProgram? (I mean that the use_sx or cx_constructor are set by OCP or the add method needs to know already...)

EDIT: Ok, this is what you eventually did by using Container instead of List. :)

Done.

---

bioptim/optimization/non_linear_program.py line 450 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

How long this "hack" lasted? hahahaha

Done.

[EveCharbie]

Reviewable status: 36 of 37 files reviewed, 8 unresolved discussions (waiting on @pariterre)

---

bioptim/dynamics/integrator.py line 65 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

We could, but this was to be consistent with Ipopt nomenclature (as "controls" does not exist for Ipopt only parameters. That said, I always found it confusing

Done.

---

bioptim/interfaces/interface_utils.py line 244 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

Is there a reason here why PenaltyHelpers.parameters does not work or is it just to reduce the amount of duplicated code? If it is the latter, I think we could argue that instead of discarding the p some line later, we could just retrieve it and remove the current line

I meant here:
t0_tp, x_tp, u_tp, _, a_tp, weight_tp, target_tp = _get_weighted_function_inputs(
penalty, idx, ocp, nlp, scaled
)

Done.

---

bioptim/limits/penalty.py line 1430 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

What if parameters changed the computation of qddot (changing the gravity for instance)?

I fixed it, but then all mx_to_cx functions in penalty, objectives and constraints should also depend on parameters which is not the case at the moment.

---

bioptim/optimization/optimization_vector.py line 442 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

the i_param does not seem to be used, did you mean ocp.parameters.keys()?

Done.

---

bioptim/optimization/parameters.py line 251 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

use_sx=True?

overwritten anyway by initialize

---

bioptim/optimization/variable_scaling.py line 27 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

I think it refers to the fact that one-column matrix are 2d arrays (like what we are doing in prior with the np.newaxis. I do not think it means it should be a matrix with more than one column though

Done.

[EveCharbie]

@pariterre our fix for Acados did not work. Can I leave it to you to fix it?

[pariterre]

@EveCharbie Yes, I am on my linux laptop today

[codecov]

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (e54be49) 78.32% compared to head (7b772fa) 78.25%.
Report is 6 commits behind head on master.

Files	Patch %	Lines
bioptim/optimization/parameters.py	81.66%	22 Missing ⚠️
...optimization/stochastic_optimal_control_program.py	16.66%	5 Missing ⚠️
bioptim/dynamics/dynamics_functions.py	63.63%	4 Missing ⚠️
bioptim/optimization/variable_scaling.py	25.00%	3 Missing ⚠️
bioptim/limits/objective_functions.py	77.77%	2 Missing ⚠️
bioptim/interfaces/acados_interface.py	95.45%	1 Missing ⚠️
bioptim/interfaces/interface_utils.py	87.50%	1 Missing ⚠️
bioptim/optimization/non_linear_program.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #846      +/-   ##
==========================================
- Coverage   78.32%   78.25%   -0.07%     
==========================================
  Files         140      139       -1     
  Lines       16232    16232              
==========================================
- Hits        12714    12703      -11     
- Misses       3518     3529      +11     

Flag	Coverage Δ
unittests	78.25% <85.00%> (-0.07%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.