[RTR] factoring out the linear continuous duplication of controls

[Ipuch]

• [RTM when tests pass] feat: Custom vector layout #1002

• Because the layout of controls is built on a duplication of controls when they are linear continuous, then I have to make sure I understand well the mechanism.

• This operation is done at several places in the code, so we need to factor out that operation to make it clean.

---

This change is 

[Ipuch]

@EveCharbie ready to review, okay to do it asynchronously.

The merge-base changed after approval.

[pariterre]

@pariterre reviewed 3 of 5 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @EveCharbie)

---

bioptim/misc/enums.py line 145 at r2 (raw file):

        return self in (ControlType.CONSTANT_WITH_LAST_NODE, ControlType.LINEAR_CONTINUOUS)

    def displayable_nodes(self, no_successor_phase: bool, end_node: bool) -> int:

What is the point of the input parameters?

---

bioptim/misc/enums.py line 145 at r2 (raw file):

        return self in (ControlType.CONSTANT_WITH_LAST_NODE, ControlType.LINEAR_CONTINUOUS)

    def displayable_nodes(self, no_successor_phase: bool, end_node: bool) -> int:

From where it was moved from, I am not sure "displayable" is actually a relevant name... It will be used for much more than display.. Not sure what name to give though

---

bioptim/misc/enums.py line 145 at r2 (raw file):

        return self in (ControlType.CONSTANT_WITH_LAST_NODE, ControlType.LINEAR_CONTINUOUS)

    def displayable_nodes(self, no_successor_phase: bool, end_node: bool) -> int:

While it is possible to add methods to Enum, we must take great care. If the class becomes large enough at some point it may become interesting to inherit from it, but it is an undefined behavior to inherit from a class that inherits from Enum. So I tend to keep these Enum as small as possible. That said, this method is okay here, it is just a reminder

---

bioptim/optimization/optimization_vector.py line 322 at r2 (raw file):

    @staticmethod
    def control_duplication(controls: dict, nlps: list["NonLinearProgram"]) -> dict:

Duplication can have two meaning here as we send a dict and a list. Is that to deal with redundant nodes or to deal with same control that appear twice?

---

bioptim/optimization/optimization_vector.py line 329 at r2 (raw file):

                for node in range(nlp.n_controls_nodes):

Black?

---

bioptim/optimization/optimization_vector.py line 331 at r2 (raw file):

                    # NOTE: hardcoded that phases are sequential 0->1->2 ... not 0->2->3 + 0->1
                    last_phase = p == (len(nlps) - 1)

is_last_phase?

---

bioptim/optimization/optimization_vector.py line 332 at r2 (raw file):

                    # NOTE: hardcoded that phases are sequential 0->1->2 ... not 0->2->3 + 0->1
                    last_phase = p == (len(nlps) - 1)
                    last_node = node == (nlp.n_controls_nodes - 1)

is_last_node?

---

bioptim/optimization/optimization_vector.py line 336 at r2 (raw file):

                    # NOTE: only different of 1 for ControlType.LINEAR_CONTINUOUS
                    n_cols = nlp.control_type.displayable_nodes(
                        no_successor_phase=last_phase,

Same question as before. I think this is legacy from when all the penalty functions were not automatically defined. Now these extra parameters are not used anymore (I may be wrong if it is to follow a Protocol share between controls and states)

[EveCharbie]

Sorry, I started reviewing before Pariterre published his comments. I'll leave mines, but you can do what you want with them :)

@EveCharbie reviewed 1 of 5 files at r1, 2 of 4 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Ipuch)

---

bioptim/optimization/optimization_vector.py line 280 at r2 (raw file):

            nu = nlp.controls.shape
            data_control_temp_phase = []
            for node in range(nlp.n_controls_nodes):  # Using n_states_nodes on purpose see higher

I think this comment can be removed

Code quote:

# Using n_states_nodes on purpose see higher

---

bioptim/optimization/optimization_vector.py line 281 at r2 (raw file):

            data_control_temp_phase = []
            for node in range(nlp.n_controls_nodes):  # Using n_states_nodes on purpose see higher
                data_control_temp_phase += [v_array[offset : offset + nu, None]]

why None ?

Code quote:

v_array[offset : offset + nu, None]

---

bioptim/optimization/optimization_vector.py line 293 at r3 (raw file):

            for node in range(nlp.n_controls_nodes):
                for key in nlp.controls.keys():
                    data_controls[p][key][node] = data_controls_temp[p][node][nlp.controls.key_index(key), :]

It seems weird to change the order of p, key and node. Is it possible to keep the same order ?

Code quote:

data_controls[p][key][node] = data_controls_temp[p][node][nlp.controls.key_index(key), :]

---

bioptim/misc/enums.py line 145 at r2 (raw file):

        return self in (ControlType.CONSTANT_WITH_LAST_NODE, ControlType.LINEAR_CONTINUOUS)

    def displayable_nodes(self, no_successor_phase: bool, end_node: bool) -> int:

Here are some ideas:
displayable_nodes -> nb_displayable_control_nodes
no_successor_phase -> has_a_successor_phase: bool
end_node -> is_final_node: bool

[Ipuch]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @EveCharbie and @pariterre)

---

bioptim/misc/enums.py line 145 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

While it is possible to add methods to Enum, we must take great care. If the class becomes large enough at some point it may become interesting to inherit from it, but it is an undefined behavior to inherit from a class that inherits from Enum. So I tend to keep these Enum as small as possible. That said, this method is okay here, it is just a reminder

Done.

---

bioptim/misc/enums.py line 145 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

From where it was moved from, I am not sure "displayable" is actually a relevant name... It will be used for much more than display.. Not sure what name to give though

yes, there is three ways of using the duplicated controls for linear continuous control in the code:

• penalties
• re-integrating of solutions
• graphs

But none of theses justified the term n_controls_steps in the nlp class, that would refer more to decision variables to me.
It is confusing because, the solver doesn't need the duplicated symbolics at all. It confused me in #1002.

That's why I've gone to a definition displayable for graphs, but it might be partially right.

---

bioptim/misc/enums.py line 145 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

What is the point of the input parameters?

Because linear continuous controls take by default in its object solution two steps in the last node of multiphase problems, except for the last phase. or with no successor phases, but I am not sure at this stage bioptim handle well to branch out for multi-scenario problems.

---

bioptim/misc/enums.py line 145 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Here are some ideas:
displayable_nodes -> nb_displayable_control_nodes
no_successor_phase -> has_a_successor_phase: bool
end_node -> is_final_node: bool

Done

---

bioptim/optimization/optimization_vector.py line 322 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Duplication can have two meaning here as we send a dict and a list. Is that to deal with redundant nodes or to deal with same control that appear twice?

with redundant nodes

---

bioptim/optimization/optimization_vector.py line 329 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Black?

done

---

bioptim/optimization/optimization_vector.py line 331 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

is_last_phase?

Done

---

bioptim/optimization/optimization_vector.py line 332 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

is_last_node?

Done

---

bioptim/optimization/optimization_vector.py line 336 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Same question as before. I think this is legacy from when all the penalty functions were not automatically defined. Now these extra parameters are not used anymore (I may be wrong if it is to follow a Protocol share between controls and states)

The question is why do we need u at the beginning and the end and of the phase in penalties of linear controls. This is because we wanted to integrate cost function with trapz for controls.

---

bioptim/optimization/optimization_vector.py line 293 at r3 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

It seems weird to change the order of p, key and node. Is it possible to keep the same order ?

Done

[Ipuch]

Reviewable status: 5 of 7 files reviewed, 13 unresolved discussions (waiting on @EveCharbie and @pariterre)

---

bioptim/optimization/optimization_vector.py line 280 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

I think this comment can be removed

Done

---

bioptim/optimization/optimization_vector.py line 281 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

why None ?

To get a vertical array to hstack afterward

[pariterre]

@pariterre reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EveCharbie)