[RTM] Pull master in crazydynamics

[Ipuch]

• blacked
• all tests works
• implicit muscle driven handled with rigidbody dynamics enum

---

This change is 

[codecov]

Codecov Report

Merging #490 (dc2a940) into CrazyDynamics (5be6662) will decrease coverage by 0.17%.
The diff coverage is 43.58%.

@@                Coverage Diff                @@
##           CrazyDynamics     #490      +/-   ##
=================================================
- Coverage          83.88%   83.71%   -0.18%     
=================================================
  Files                 86       86              
  Lines               9354     9407      +53     
=================================================
+ Hits                7847     7875      +28     
- Misses              1507     1532      +25     

Impacted Files	Coverage Δ
bioptim/examples/acados/pendulum.py	84.21% <0.00%> (+2.15%)	⬆️
bioptim/examples/acados/static_arm.py	42.85% <0.00%> (ø)
bioptim/examples/fatigue/pendulum_with_fatigue.py	82.60% <0.00%> (ø)
...ioptim/examples/fatigue/static_arm_with_fatigue.py	67.85% <0.00%> (ø)
...amples/getting_started/custom_phase_transitions.py	92.75% <0.00%> (ø)
...mples/getting_started/example_implicit_dynamics.py	89.47% <0.00%> (ø)
...s/getting_started/example_multinode_constraints.py	90.16% <0.00%> (ø)
.../examples/getting_started/example_save_and_load.py	64.10% <0.00%> (ø)
.../examples/moving_horizon_estimation/cyclic_nmpc.py	63.88% <0.00%> (ø)
...les/moving_horizon_estimation/multi_cyclic_nmpc.py	66.66% <0.00%> (ø)
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5be6662...dc2a940. Read the comment docs.

[pariterre]

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

---

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

            If the dynamic with contact should be used
        rigidbody_dynamics: RigidBodyDynamics
            which rigidbody dynamics should be used (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)

Is it RigidBodyDynamics.ODE or (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)? There is a mismatch with the parameter of the function

Code quote:

        rigidbody_dynamics: RigidBodyDynamics
            which rigidbody dynamics should be used (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)

---

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

            rigidbody_dynamics == RigidBodyDynamics.DAE_FORWARD_DYNAMICS
            or rigidbody_dynamics == RigidBodyDynamics.DAE_FORWARD_DYNAMICS_JERK
            or rigidbody_dynamics == RigidBodyDynamics.DAE_INVERSE_DYNAMICS_JERK

Should not this be a if not ... and put what is allowed in the if. So we are making sure that if someone ever add a new dynamics this dynamics won't automatically be accepted (since it won't fail here)

Code quote:

            rigidbody_dynamics == RigidBodyDynamics.DAE_FORWARD_DYNAMICS
            or rigidbody_dynamics == RigidBodyDynamics.DAE_FORWARD_DYNAMICS_JERK
            or rigidbody_dynamics == RigidBodyDynamics.DAE_INVERSE_DYNAMICS_JERK
        ):
            raise NotImplementedError("MUSCLE_DRIVEN cannot be used with this enum RigidBodyDynamics yet")

---

bioptim/dynamics/dynamics_functions.py line 407 at r1 (raw file):

            If the dynamic with contact should be used
        rigidbody_dynamics: RigidBodyDynamics
            which rigidbody dynamics should be used (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)

Please make sure the () are legit... actually I see no reason (apart from telling what is allowed to have anything explained in parenthesis here

Code quote:

        rigidbody_dynamics: RigidBodyDynamics
            which rigidbody dynamics should be used (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)

---

bioptim/examples/muscle_driven_ocp/muscle_activations_tracker.py line 102 at r1 (raw file):

            symbolic_parameters,
            nlp,
            False,

please add the hints (something=something) so we understand the False refers to what

Code quote:

            nlp,
            False,
            RigidBodyDynamics.ODE,

---

bioptim/examples/muscle_driven_ocp/muscle_excitations_tracker.py line 109 at r1 (raw file):

            nlp,
            False,
            RigidBodyDynamics.ODE,

Isn't it by default? If not, it should be

---

bioptim/interfaces/acados_interface.py line 664 at r1 (raw file):

            self.opts.set_only_first_options_has_changed(False)
            self.opts.set_has_tolerance_changed(False)

Please make sure to commit these kind of improvements in a separate and very fast to review/accept PR. The longer these lies in a PR, the more chances it creates conflict with other PR, especially in files that don't matter to you

---

bioptim/interfaces/solver_options.py line 370 at r1 (raw file):

        def set_option(self, val, name):
            if f"_{name}" not in self.__dict__.keys():
                self.__dict__[f"_{name}"] = val

This seems highly dangerous! Why would you only set an option if it was not already there and ignore the set otherwise? Also, what if this new key does not exist in Ipopt, we offer no waranty here... If you need this for some sort of hacking behind the bioptim wall, please be very specific in the title of the function (set_option_unsafe or something like that)

Code quote:

            if f"_{name}" not in self.__dict__.keys():
                self.__dict__[f"_{name}"] = val

---

bioptim/interfaces/solver_options.py line 464 at r1 (raw file):

        def set_option(self, val: Union[float, int, str], name: str):
            if f"_{name}" not in self.__annotations__.keys():
                self.__annotations__[f"_{name}"] = val

This seems highly dangerous! Why would you only set an option if it was not already there and ignore the set otherwise? Also, what if this new key does not exist in Ipopt, we offer no waranty here... If you need this for some sort of hacking behind the bioptim wall, please be very specific in the title of the function (set_option_unsafe or something like that)

Code quote:

        def set_option(self, val: Union[float, int, str], name: str):
            if f"_{name}" not in self.__annotations__.keys():
                self.__annotations__[f"_{name}"] = val
                self.__setattr__(f"_{name}", val)
                self.set_only_first_options_has_changed(True)

---

bioptim/interfaces/solver_options.py line 570 at r1 (raw file):

        def set_convergence_tolerance(self, val: Union[float, int, list, tuple]):
            if isinstance(val, (float, int)):

I disagree with this. I understand why you made that, but what if someone change the order in the future. All the setter bellow can be called individually. There is no reason to force the user to know what order we decided (emphasised by the fact that there is no documentation on the method, which is normal because of the the fact it is an overloaded method if I recall well). Moreover, changing the signature of an overloaded method is bad practise (I did not open your PR in an editor, but this is probably overlighted saying exactly this). Please remove

Code quote:

        def set_convergence_tolerance(self, val: Union[float, int, list, tuple]):
            if isinstance(val, (float, int)):
                val = [val] * 4

---

bioptim/interfaces/solver_options.py line 601 at r1 (raw file):

            return self._nlp_solver_max_iter

        def set_maximum_iterations(self, num: int):

Please change this in the base class too

[Ipuch]

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

---

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

Previously, pariterre (Pariterre) wrote…

Is it RigidBodyDynamics.ODE or (EXPLICIT, IMPLICIT, SEMI_EXPLICIT)? There is a mismatch with the parameter of the function

Removed.

---

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

Previously, pariterre (Pariterre) wrote…

Should not this be a if not ... and put what is allowed in the if. So we are making sure that if someone ever add a new dynamics this dynamics won't automatically be accepted (since it won't fail here)

Not sure what you meant.

---

bioptim/dynamics/dynamics_functions.py line 407 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please make sure the () are legit... actually I see no reason (apart from telling what is allowed to have anything explained in parenthesis here

removed

---

bioptim/examples/muscle_driven_ocp/muscle_activations_tracker.py line 102 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

please add the hints (something=something) so we understand the False refers to what

Done

---

bioptim/examples/muscle_driven_ocp/muscle_excitations_tracker.py line 109 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Isn't it by default? If not, it should be

it was not now done.

---

bioptim/interfaces/acados_interface.py line 664 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please make sure to commit these kind of improvements in a separate and very fast to review/accept PR. The longer these lies in a PR, the more chances it creates conflict with other PR, especially in files that don't matter to you

I'm not sure where it comes from. It's already pushed on the master.
I just merged the master on my branch to get updated.

---

bioptim/interfaces/solver_options.py line 370 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

This seems highly dangerous! Why would you only set an option if it was not already there and ignore the set otherwise? Also, what if this new key does not exist in Ipopt, we offer no waranty here... If you need this for some sort of hacking behind the bioptim wall, please be very specific in the title of the function (set_option_unsafe or something like that)

Same, this comes from another PR.

---

bioptim/interfaces/solver_options.py line 464 at r1 (raw file):

Same, this comes from another PR.

---

bioptim/interfaces/solver_options.py line 570 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

I disagree with this. I understand why you made that, but what if someone change the order in the future. All the setter bellow can be called individually. There is no reason to force the user to know what order we decided (emphasised by the fact that there is no documentation on the method, which is normal because of the the fact it is an overloaded method if I recall well). Moreover, changing the signature of an overloaded method is bad practise (I did not open your PR in an editor, but this is probably overlighted saying exactly this). Please remove

Same, this comes from another PR.

---

bioptim/interfaces/solver_options.py line 601 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Please change this in the base class too

Same, this comes from another PR. I think a new PR, should be open on the master to fix all theses related issues.

[pariterre]

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

---

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

Previously, Ipuch (Pierre Puchaud) wrote…

Not sure what you meant.

We discussed verbally

---

bioptim/dynamics/dynamics_functions.py line 144 at r1 (raw file):

        defects = None
        # todo: contacts and fatigue to be handled with implicit dynamics
        if not with_contact and fatigue is None:

Is this intended?

[Ipuch]

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

---

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

Previously, pariterre (Pariterre) wrote…

We discussed verbally

done

---

bioptim/dynamics/dynamics_functions.py line 144 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Is this intended?

no. restored

---

bioptim/interfaces/solver_options.py line 370 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Same, this comes from another PR.

in #493

---

bioptim/interfaces/solver_options.py line 464 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Same, this comes from another PR.

in #493

---

bioptim/interfaces/solver_options.py line 570 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Same, this comes from another PR.

in #493

---

bioptim/interfaces/solver_options.py line 601 at r1 (raw file):

Please change this in the base class too
in #493

[Ipuch]

RTM ?

[pariterre]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Ipuch)

---

bioptim/dynamics/configure_problem.py line 407 at r3 (raw file):

        if (
                not rigidbody_dynamics == RigidBodyDynamics.DAE_INVERSE_DYNAMICS

this works but is a bit unusal. We usually use != instead of noting the ==
Could you change it?

Code quote:

not rigidbody_dynamics == RigidBodyDynamics.DAE_INVERSE_DYNAMICS

[Ipuch]

Tests should work fine now

[pariterre]

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

[Ipuch]

@pariterre mergeable?