[RTM] collocation initial guess

[thasaarah]

This change is 

related to #406

[pariterre]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @thasaarah)

---

bioptim/examples/getting_started/custom_initial_guess.py line 139 at r1 (raw file):

        if ode_solver.is_direct_collocation:
            x = np.random.random((nq + nqdot, n_shooting * (ode_solver.polynomial_degree + 1) + 1))
            u = np.random.random((ntau, n_shooting))

Please remove the if check here. We want to let the capability to the example to fail if one wants to try this ALL_POINTS thing

Code quote:

        if ode_solver.is_direct_collocation:
            x = np.random.random((nq + nqdot, n_shooting * (ode_solver.polynomial_degree + 1) + 1))
            u = np.random.random((ntau, n_shooting))
        else:
            raise ValueError("InterpolationType.ALL_POINTS must only be used in direct collocation")

---

bioptim/limits/path_conditions.py line 248 at r1 (raw file):

            if self.shape[1] != self.n_shooting:
                raise RuntimeError(
                    f"Invalid number of column for InterpolationType.EACH_FRAME (ncols = {self.shape[1]}), "

ALL_POINTS

---

bioptim/misc/enums.py line 50 at r1 (raw file):

    LINEAR = 2  # Linear interpolation between first and last
    EACH_FRAME = 3  # Each values are provided by the user
    ALL_POINTS = 4

Please add a relevent description (and in the ReadMe too)

---

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

            if type(self.ocp.original_values["x_init"]) == InitialGuess:
                interpolation_type = nlp.x_init.type
            elif type(self.ocp.original_values["x_init"]) == InitialGuessList:

What is the expected value of interpolation_type if both if fails?

Code quote:

            if type(self.ocp.original_values["x_init"]) == InitialGuess:
                interpolation_type = nlp.x_init.type
            elif type(self.ocp.original_values["x_init"]) == InitialGuessList:
                interpolation_type = self.ocp.original_values["x_init"][phase].type

---

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

            if ocp.nlp[i].x_init.type == InterpolationType.ALL_POINTS:
                if not ocp.nlp[i].ode_solver.is_direct_collocation:
                    raise ValueError("InterpolationType.ALL_POINTS must only be used in direct collocation")

"with" direct collocation

---

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

                if not ocp.nlp[i].ode_solver.is_direct_collocation:
                    raise ValueError("InterpolationType.ALL_POINTS must only be used in direct collocation")
            if hasattr(ocp.original_values["x_init"], "type"):

hasattr is usually a design flaw. Please create the define the value no matter what with a default to None.

That said, it seems that you are testing for something else indirectly. Do not do that. Test directly and explicitely what you need

Code quote:

            if hasattr(ocp.original_values["x_init"], "type"):
                interpolation_type = ocp.original_values["x_init"].type
            elif hasattr(ocp.original_values["x_init"], "options"):
                interpolation_type = ocp.original_values["x_init"].options[i][0].type
            else:
                interpolation_type = None

---

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

            if (
                nlp.ode_solver.is_direct_collocation
                and interpolation_type != InterpolationType.EACH_FRAME

Do you mean == InterpolationType.ALL_POINTS? If so, please be explicit

[thasaarah]

Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @pariterre)

---

bioptim/examples/getting_started/custom_initial_guess.py line 139 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please remove the if check here. We want to let the capability to the example to fail if one wants to try this ALL_POINTS thing

Done.

---

bioptim/limits/path_conditions.py line 248 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

ALL_POINTS

Done.

---

bioptim/misc/enums.py line 50 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please add a relevent description (and in the ReadMe too)

Done.

---

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

Previously, pariterre (Pariterre) wrote…

What is the expected value of interpolation_type if both if fails?

None

---

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

Previously, pariterre (Pariterre) wrote…

"with" direct collocation

Done.

---

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

Previously, pariterre (Pariterre) wrote…

hasattr is usually a design flaw. Please create the define the value no matter what with a default to None.

That said, it seems that you are testing for something else indirectly. Do not do that. Test directly and explicitely what you need

Done.

---

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

Previously, pariterre (Pariterre) wrote…

Do you mean == InterpolationType.ALL_POINTS? If so, please be explicit

No, any interpolation type but EACH_FRAME

[codecov]

Codecov Report

Base: 80.49% // Head: 83.99% // Increases project coverage by +3.49% 🎉

Coverage data is based on head (ee7b9d5) compared to base (e831e3a).
Patch coverage: 90.08% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   80.49%   83.99%   +3.49%     
==========================================
  Files          87       88       +1     
  Lines        9552     9770     +218     
==========================================
+ Hits         7689     8206     +517     
+ Misses       1863     1564     -299     

Impacted Files	Coverage Δ
bioptim/optimization/solution.py	85.56% <ø> (ø)
bioptim/gui/graph.py	95.20% <50.00%> (-0.27%)	⬇️
bioptim/dynamics/integrator.py	94.73% <69.23%> (-2.20%)	⬇️
bioptim/dynamics/configure_problem.py	90.07% <77.27%> (-1.58%)	⬇️
...mples/getting_started/example_implicit_dynamics.py	89.47% <77.27%> (+44.54%)	⬆️
bioptim/dynamics/dynamics_functions.py	96.32% <87.50%> (-1.85%)	⬇️
bioptim/limits/path_conditions.py	85.29% <93.61%> (+0.94%)	⬆️
bioptim/limits/constraints.py	83.63% <97.50%> (+6.55%)	⬆️
bioptim/__init__.py	100.00% <100.00%> (ø)
bioptim/dynamics/dynamics_evaluation.py	100.00% <100.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pariterre]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @thasaarah)

---

bioptim/limits/path_conditions.py line 898 at r2 (raw file):

            initial_guess=self.noised_initial_guess,
            interpolation=interpolation
            if interpolation == InterpolationType.ALL_POINTS

Please add a comment that says that this interpolation should always be done at each data point

Code quote:

            interpolation=interpolation
            if interpolation == InterpolationType.ALL_POINTS
            else InterpolationType.EACH_FRAME,

---

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

Previously, thasaarah wrote…

Done.

type(...) == SomeClass should be isinstance(..., SomeClass)

---

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

                interpolation_type = self.ocp.original_values["x_init"][phase].type
            else:
                interpolation_type = None

Please make sure this line is tested, if not, check if it is sane (do we allow "x_init" to not be either). If it runs smoothly then change interpolation_type = None for pass, if it fails, raise an error.

Also add a line return after this if block

Code quote:

 interpolation_type = None

---

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

                interpolation_type = ocp.original_values["x_init"][i].type
            else:
                interpolation_type = None

Same thing as before

Add a line return after this if block

---

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

                if ocp.nlp[i].ode_solver.is_direct_collocation
                and interpolation_type != InterpolationType.EACH_FRAME
                and type(ocp.nlp[i].x_init) != NoisedInitialGuess

isinstance instead of type (if it makes sense)

Code quote:

type(ocp.nlp[i].x_init) != NoisedInitialGuess

---

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

                for p in range(repeat if k != nlp.ns else 1):
                    span = slice(k * outer_offset + p * nx, k * outer_offset + (p + 1) * nx)
                    if nlp.x_init.type == InterpolationType.EACH_FRAME and type(nlp.x_init) != NoisedInitialGuess:

isinstance

Code quote:

ype(nlp.x_init) != NoisedInitialGuess:

[thasaarah]

Reviewable status: 4 of 11 files reviewed, 6 unresolved discussions (waiting on @pariterre and @thasaarah)

---

bioptim/limits/path_conditions.py line 898 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Please add a comment that says that this interpolation should always be done at each data point

Done.

---

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

Previously, pariterre (Pariterre) wrote…

type(...) == SomeClass should be isinstance(..., SomeClass)

Done.

---

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

Previously, pariterre (Pariterre) wrote…

Same thing as before

Add a line return after this if block

Done.

---

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

Previously, pariterre (Pariterre) wrote…

isinstance instead of type (if it makes sense)

used type(ocp.nlp[i].x_init) is InitialGuess here to make the condition more evident by avoiding negation

---

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

Previously, pariterre (Pariterre) wrote…

isinstance

Done.

[thasaarah]

Reviewable status: 4 of 11 files reviewed, 6 unresolved discussions (waiting on @pariterre)

---

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

Previously, pariterre (Pariterre) wrote…

Please make sure this line is tested, if not, check if it is sane (do we allow "x_init" to not be either). If it runs smoothly then change interpolation_type = None for pass, if it fails, raise an error.

Also add a line return after this if block

interpolation_type needs to defined before a value can be assigned to ns

[pariterre]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pariterre and @thasaarah)

---

bioptim/limits/path_conditions.py line 898 at r2 (raw file):

Previously, thasaarah wrote…

Done.

It was added but at the wrong place, it should be with the if

---

bioptim/limits/path_conditions.py line 894 at r4 (raw file):

        if bounds is None:
            raise RuntimeError("bounds must be specified to generate noised initial guess")

Add single quotations around 'bounds' to emphasize it is a parameter

Code quote:

bounds

---

bioptim/limits/path_conditions.py line 900 at r4 (raw file):

            bounds.n_shooting = initial_guess.shape[1]
            bounds.min.n_shooting = initial_guess.shape[1]
            bounds.max.n_shooting = initial_guess.shape[1]

The min and max update should be automatically done in Bounds if n_shooting is modify. So :

bounds.n_shooting = value 

should be enough

Code quote:

            bounds.n_shooting = initial_guess.shape[1]
            bounds.min.n_shooting = initial_guess.shape[1]
            bounds.max.n_shooting = initial_guess.shape[1]

---

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

        if ocp.nlp[phase].ode_solver.is_direct_collocation:
            if (
                isinstance(ocp.nlp[phase].x_init, NoisedInitialGuess)

Is suggestion good?

Suggestion:

        ns = ocp.nlp[phase].ns
        if ocp.nlp[phase].ode_solver.is_direct_collocation:
            if isinstance(ocp.nlp[phase].x_init, NoisedInitialGuess):
                if interpolation_type == InterpolationType.ALL_POINTS:
                    ns *= (ocp.nlp[phase].ode_solver.steps + 1)
            elif isinstance(ocp.nlp[phase].x_init, InitialGuess):
                if interpolation_type != InterpolationType.EACH_FRAME:
                    ns *= (ocp.nlp[phase].ode_solver.steps + 1)
        return ns

---

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

                    if type(nlp.x_init) is InitialGuess and nlp.x_init.type == InterpolationType.EACH_FRAME:
                        point = k * repeat + p
                    elif isinstance(nlp.x_init, NoisedInitialGuess) and nlp.x_init.type == InterpolationType.ALL_POINTS:

Same as before, put isinstance(..., Noised...) before the instance(..., InitialGuess) so it catches it before it can be catch be InitialGuess. Therefore no need for type == ...

Code quote:

isinstance(nlp.x_init, NoisedInitialGuess)

---

external/acados line 0 at r4 (raw file):
This is not acceptable, please force push to reinstate

[pariterre]

Reviewed 8 of 41 files at r8, 35 of 35 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pariterre and @thasaarah)

---

bioptim/limits/path_conditions.py line 518 at r9 (raw file):

        return self.min.shape

    # todo: to be removed?

nope, n_shooting should be changed to _n_shooting with a property that returns _n_shooting and a setter. This would look like this:

def __init__(self):
    self._n_shooting = ns

    @property
    def n_shooting(self):
        return self._n_shooting

    @n_shooting.setter
    def n_shooting(self, ns):
        self._n_shooting = ns
        self.min.n_shooting = ns
        self.max.n_shooting = ns

[pariterre]

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @pariterre and @thasaarah)

---

bioptim/limits/path_conditions.py line 908 at r10 (raw file):

            interpolation = initial_guess.type
        if interpolation == InterpolationType.ALL_POINTS:
            bounds._n_shooting = initial_guess.shape[1]

this should be bounds.n_shooting

---

tests/test_update_bounds_and_init.py line 1444 at r10 (raw file):

            ]
        )
    elif interpolation == InterpolationType.CONSTANT_WITH_FIRST_AND_LAST_DIFFERENT:

These two tests should be there

[thasaarah]

Reviewable status: 44 of 45 files reviewed, 6 unresolved discussions (waiting on @pariterre)

---

bioptim/limits/path_conditions.py line 898 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

It was added but at the wrong place, it should be with the if

Done.

---

bioptim/limits/path_conditions.py line 894 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

Add single quotations around 'bounds' to emphasize it is a parameter

Done.

---

bioptim/limits/path_conditions.py line 900 at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

The min and max update should be automatically done in Bounds if n_shooting is modify. So :

bounds.n_shooting = value 

should be enough

added a function to set n_shooting but it might not be necessary

---

bioptim/limits/path_conditions.py line 518 at r9 (raw file):

Previously, pariterre (Pariterre) wrote…

nope, n_shooting should be changed to _n_shooting with a property that returns _n_shooting and a setter. This would look like this:

def __init__(self):
    self._n_shooting = ns

    @property
    def n_shooting(self):
        return self._n_shooting

    @n_shooting.setter
    def n_shooting(self, ns):
        self._n_shooting = ns
        self.min.n_shooting = ns
        self.max.n_shooting = ns

Done.

---

bioptim/limits/path_conditions.py line 908 at r10 (raw file):

Previously, pariterre (Pariterre) wrote…

this should be bounds.n_shooting

Done.

---

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

Previously, pariterre (Pariterre) wrote…

Is suggestion good?

yes

---

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

Previously, pariterre (Pariterre) wrote…

Same as before, put isinstance(..., Noised...) before the instance(..., InitialGuess) so it catches it before it can be catch be InitialGuess. Therefore no need for type == ...

Done.

---

external/acados line at r4 (raw file):

Previously, pariterre (Pariterre) wrote…

This is not acceptable, please force push to reinstate

Done.

---

tests/test_update_bounds_and_init.py line 1444 at r10 (raw file):

Previously, pariterre (Pariterre) wrote…

These two tests should be there

i think it was already fixed

[pariterre]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pariterre)

[Ipuch]

RTM ?

[pariterre]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pariterre)

[pariterre]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pariterre)