[RTM] Continuity objective take2

[Sciancisco]

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?
• 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 is a second take on PR #480 .
Will write test once example is set in stone.

---

This change is 

[codecov]

Codecov Report

Base: 84.02% // Head: 83.67% // Decreases project coverage by -0.35% ⚠️

Coverage data is based on head (cebff01) compared to base (5105f1c).
Patch coverage: 64.47% of modified lines in pull request are covered.

❗ Current head cebff01 differs from pull request most recent head f1e4737. Consider uploading reports for the commit f1e4737 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   84.02%   83.67%   -0.36%     
==========================================
  Files          88       89       +1     
  Lines        9796     9909     +113     
==========================================
+ Hits         8231     8291      +60     
- Misses       1565     1618      +53     

Impacted Files	Coverage Δ
bioptim/dynamics/configure_problem.py	90.07% <ø> (ø)
...getting_started/example_continuity_as_objective.py	47.43% <47.43%> (ø)
bioptim/limits/objective_functions.py	88.60% <72.72%> (-5.15%)	⬇️
bioptim/limits/constraints.py	83.75% <93.75%> (+0.11%)	⬆️
bioptim/limits/multinode_constraint.py	91.72% <100.00%> (-0.19%)	⬇️
bioptim/limits/penalty_option.py	88.88% <100.00%> (ø)
bioptim/limits/phase_transition.py	93.02% <100.00%> (ø)
bioptim/misc/enums.py	100.00% <100.00%> (ø)
bioptim/optimization/optimal_control_program.py	87.64% <100.00%> (+0.04%)	⬆️

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 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Sciancisco)

---

bioptim/examples/getting_started/example_as_objective.py line 3 at r1 (raw file):

"""
TODO: General cleaning
A very simple yet meaningful optimal control program consisting in a pendulum starting downward and ending upward

It does look like a "Very simple" example to me!

Code quote:

A very simple yet meaningful

---

bioptim/examples/getting_started/example_as_objective.py line 87 at r1 (raw file):

    # Path constraint
    Ytrans = 0

please use camel_case

---

bioptim/examples/getting_started/example_as_objective.py line 89 at r1 (raw file):

    Ytrans = 0
    Xrot = 1
    START = 0

Do not use capitalized variables

---

bioptim/examples/getting_started/example_as_objective.py line 180 at r1 (raw file):

    # Path constraint
    Ytrans = 0

Please use camel_case

Code quote:

    # Path constraint
    Ytrans = 0
    Xrot = 1
    START = 0
    END = -1

---

bioptim/examples/getting_started/example_as_objective.py line 187 at r1 (raw file):

    x_bounds = QAndQDotBounds(biorbd_model)
    x_bounds[:, START] = 0
    x_bounds.min[Ytrans, END] = -0.01  # Give a little slack on the end position, otherwise to difficult

too?

Code quote:

to

---

bioptim/examples/getting_started/example_as_objective.py line 204 at r1 (raw file):

    constraints = ConstraintList()
    # max_bound is practically at infinity

I think np.inf actually works

Code quote:

 # max_bound is practically at infinity

---

bioptim/examples/getting_started/example_as_objective.py line 246 at r1 (raw file):

    solver_first.set_maximum_iterations(500)

    # Custom plots

Please make sure all these graphs actually make sense

Code quote:

    # Custom plots
    ocp_first.add_plot_penalty(CostType.ALL)

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 1 at r1 (raw file):

version 4

I do not understand the name of the pendulum?

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 56 at r1 (raw file):

    mesh 0 $y1 $z1+$r1
    mesh 0 $y1+(-$r1) $z1
    mesh 0 $y1 $z1+(-$r1)  // TODO: $z-$r causes a segfault in biorbd

Good luck with that hahaha

Code quote:

// TODO: $z-$r causes a segfault in biorbd

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 90 at r1 (raw file):

    mesh 0 $y5 $z5+(-$r5)
    mesh 0 $y5+$r5 $z5
endsegment

Add a line carriage at the end of the file

---

bioptim/limits/phase_transition.py line 71 at r1 (raw file):

            min_bound=min_bound,
            max_bound=max_bound,
            weight=weight if weight else 0,

I think it works, but recently PEP8 asked to be explicit when testing for a None so
weight if weight is not None else 0

Code quote:

if weight

---

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

class PenaltyType(Enum):  # it's more of a "Category" than "Type"

I agree, I could not find a good word, it would have been a good idea. But now it is a bit too late...

Code quote:

# it's more of a "Category" than "Type"

[Sciancisco]

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

---

bioptim/examples/getting_started/example_as_objective.py line 3 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

It does look like a "Very simple" example to me!

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 87 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

please use camel_case

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 89 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Do not use capitalized variables

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 180 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please use camel_case

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 187 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

too?

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 204 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I think np.inf actually works

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 246 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please make sure all these graphs actually make sense

Meaning? Continuity does change from graph CONSTRAINTS to graph OBJECTIVES.

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 1 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I do not understand the name of the pendulum?

Originally it was supposed to be a big maze in which the pendulum would have had to navigate. Now it is a smaller "maze".

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 56 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Good luck with that hahaha

I'll leave it there.

---

bioptim/examples/getting_started/models/pendulum_maze.bioMod line 90 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Add a line carriage at the end of the file

Done.

---

bioptim/limits/phase_transition.py line 71 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I think it works, but recently PEP8 asked to be explicit when testing for a None so
weight if weight is not None else 0

Done.

---

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

Previously, pariterre (Pariterre) wrote…

I agree, I could not find a good word, it would have been a good idea. But now it is a bit too late...

Should I remove or leave for the future devs to understand better?

[pariterre]

Please make sure the tests passe and we are good to go ;)

Reviewed 13 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Sciancisco)

---

bioptim/interfaces/ipopt_interface.py line 343 at r3 (raw file):

                    else:
                        x, u = get_x_and_u_at_idx(penalty, idx)
                    # for the future bioptim adventurer: here lies the reason that a constraint has weight = 0.

has weight = 0 for what?

---

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

Previously, Sciancisco wrote…

Should I remove or leave for the future devs to understand better?

Yah, let's leave it there

---

bioptim/examples/getting_started/example_as_objective.py line 246 at r1 (raw file):

Previously, Sciancisco wrote…

Meaning? Continuity does change from graph CONSTRAINTS to graph OBJECTIVES.

I mean, this file should be a minimal example. Does it makes sense to actually show all these graphs. Maybe (probably) they are, I do not run the files while reviewing. It just seems a bit complicated as a minimal example, so I wanted to be sure

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 6 at r3 (raw file):

There is a catch however: there are regions in which the weight of the pendulum cannot go.
The problem is solved in two passes. In the first pass, continuity is an objective rather then a constraint.

Suggestion:

rather than

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 25 at r3 (raw file):

    InterpolationType,
    QAndQDotBounds,
    NoisedInitialGuess,

Is there a reason to use NoisedInitialGuess? It seems that it will confuse the users that are solely interested in the continuity as objective?

Code quote:

NoisedInitialGuess,

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 131 at r3 (raw file):

        n_threads=n_threads,
        # change the weight to observe the impact on the continuity of the solution
        # or comment to see how the constrained program would fare

I think the example should define a variable at the top of the function, because as a user when I look at the example file, I expect the relevant part to be highlighted. Right now, the most important bit feels actually burry in a highly complex OCP (I understand the scientific relevance of the example, but not so much as a minimal example...)

Code quote:

        # change the weight to observe the impact on the continuity of the solution
        # or comment to see how the constrained program would fare
        state_continuity_weight=1000000,

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 192 at r3 (raw file):

    constraints.add(out_of_sphere, y=-0.45, z=0, min_bound=0.35, max_bound=np.inf, node=Node.ALL_SHOOTING)
    constraints.add(out_of_sphere, y=0.05, z=0, min_bound=0.35, max_bound=np.inf, node=Node.ALL_SHOOTING)
    # HERE

What is that for?
EDIT: I just saw the comment before... Maybe add something like (referenced in the first pass)?

Code quote:

# HERE

[Sciancisco]

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

---

bioptim/interfaces/ipopt_interface.py line 343 at r3 (raw file):

Previously, pariterre (Pariterre) wrote…

has weight = 0 for what?

Moved the comment in PenaltyOption in _set_penalty_function where it is more relevant. Remember when I wanted to change the behaviour of weight = 0. This is to warn that it is much more fundamental than one might think.

---

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

Previously, pariterre (Pariterre) wrote…

Yah, let's leave it there

Done.

---

bioptim/examples/getting_started/example_as_objective.py line 246 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I mean, this file should be a minimal example. Does it makes sense to actually show all these graphs. Maybe (probably) they are, I do not run the files while reviewing. It just seems a bit complicated as a minimal example, so I wanted to be sure

Done.

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 6 at r3 (raw file):

There is a catch however: there are regions in which the weight of the pendulum cannot go.
The problem is solved in two passes. In the first pass, continuity is an objective rather then a constraint.

Done.

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 25 at r3 (raw file):

Previously, pariterre (Pariterre) wrote…

Is there a reason to use NoisedInitialGuess? It seems that it will confuse the users that are solely interested in the continuity as objective?

It allows to show that the solution with continuity constraints converges but the solution is bad. Without NoisedInitialGuess, it just fails.

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 131 at r3 (raw file):

Previously, pariterre (Pariterre) wrote…

I think the example should define a variable at the top of the function, because as a user when I look at the example file, I expect the relevant part to be highlighted. Right now, the most important bit feels actually burry in a highly complex OCP (I understand the scientific relevance of the example, but not so much as a minimal example...)

Done.

---

bioptim/examples/getting_started/example_continuity_as_objective.py line 192 at r3 (raw file):

Previously, pariterre (Pariterre) wrote…

What is that for?
EDIT: I just saw the comment before... Maybe add something like (referenced in the first pass)?

Done.

[pariterre]

Reviewed 9 of 9 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Sciancisco)

[pariterre]

LTGM had not appeared!!
Still:

haha