[RTM when tests pass] external forces moved out of ocp too

[Ipuch]

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? External forces should not be an argument of OCP #729
• 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 

[pariterre]

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

---

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

        """

        _check_contacts_in_biorbd_model(with_contact, nlp.model.nb_contacts, nlp.phase_idx)

in bio_model?

---

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

            return qdot_var.mapping.to_first.map(qddot)
        else:

This (and the for loop associated) should probably not rely on this check anymore thanks to phase_dynamics...

---

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

        #             The external_forces should be of format tuple[tuple[Any]] where the outer list is the number of "
        #             "phases, the inner list is the number of shooting points of each phase and the dict is the Any is "
        #             "the specific way to add external_force for the specific implementation of the biomodel

to be removed?

---

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

        #                 "is the specific way to add external_force for the specific implementation of the biomodel"
        #             )
        #     NLP.add(self, "external_forces", external_forces, False)

Part of this check is probably needed to dispatch properly the external forces for each node?

[Ipuch]

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

---

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

Previously, pariterre (Pariterre) wrote…

This (and the for loop associated) should probably not rely on this check anymore thanks to phase_dynamics...

Do you agree with a check the common use of phase_dynamics and external_forces ?

[pariterre]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Ipuch)

---

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

Previously, Ipuch (Pierre Puchaud) wrote…

Do you agree with a check the common use of phase_dynamics and external_forces ?

I think checking for phase_dynamics make sense. A check on external_forces could be check (and raise?). I don't what I prefer... Raising has the advantage of removing adverse effect of ignoring the values not a [0], while not raising make sense went external_forces are optimized or when a single value of external_forces is actually expected

[Ipuch]

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

---

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

Previously, pariterre (Pariterre) wrote…

in bio_model?

I partly disagree because the error message has to return the phase idx where there is an error.
I partly agree if we decide to move the feature 'with_XXX' in the models that this would be directly raised in the biorbdModel.

---

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

Previously, pariterre (Pariterre) wrote…

I think checking for phase_dynamics make sense. A check on external_forces could be check (and raise?). I don't what I prefer... Raising has the advantage of removing adverse effect of ignoring the values not a [0], while not raising make sense went external_forces are optimized or when a single value of external_forces is actually expected

But I feel like there is no case where we want to apply a constant external force ...

I'm kind not satisfied with such an implentation because there is no concept of series dispatched over the phase nodes yet. This would look like this ...

        if nlp.phase_dynamics == PhaseDynamics.SHARED_DURING_THE_PHASE:
            if with_contact:
                qddot = nlp.model.constrained_forward_dynamics(q, qdot, tau)
            else:
                qddot = nlp.model.forward_dynamics(q, qdot, tau)

            return qdot_var.mapping.to_first.map(qddot)

        elif nlp.phase_dynamics == PhaseDynamics.ONE_PER_NODE:
            dxdt = MX(len(qdot_var.mapping.to_first), nlp.ns)
            for i, f_ext in enumerate(external_forces):
                if with_contact:
                    qddot = nlp.model.constrained_forward_dynamics(q, qdot, tau, f_ext)
                else:
                    qddot = nlp.model.forward_dynamics(q, qdot, tau, f_ext)
                dxdt[:, i] = qdot_var.mapping.to_first.map(qddot)
            return dxdt

        else:
            raise RuntimeError(f"PhaseDynamics {nlp.phase_dynamics} not implemented yet")

---

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

Previously, pariterre (Pariterre) wrote…

to be removed?

Done

---

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

Previously, pariterre (Pariterre) wrote…

Part of this check is probably needed to dispatch properly the external forces for each node?

Done

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
bioptim/dynamics/configure_problem.py	71.00% <100.00%> (+2.37%)	⬆️
bioptim/dynamics/dynamics_functions.py	84.29% <100.00%> (-2.25%)	⬇️
...xamples/getting_started/example_external_forces.py	87.09% <100.00%> (-0.41%)	⬇️
bioptim/optimization/non_linear_program.py	90.47% <ø> (ø)
bioptim/optimization/optimal_control_program.py	85.37% <ø> (+0.12%)	⬆️
...optimization/stochastic_optimal_control_program.py	26.00% <ø> (ø)

📢 Thoughts on this report? Let us know!.

[pariterre]

Reviewed 8 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Ipuch)

---

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

):
    """If external_forces is not None, phase_dynamics should be PhaseDynamics.ONE_PER_NODE"""
    if external_forces is not None and phase_dynamics != PhaseDynamics.ONE_PER_NODE:

add a comment to explain why even though this is not a fundamental reason why external_forces necessitate ONE_PER_NODE, we made it anyway

---

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

nlp.model.constrained_forward_dynamics

A todo should be added to pass f_ext in controls?

---

tests/shard1/test_dynamics.py line 42 at r3 (raw file):

    "with_external_force",
    [
        # False,

Reinsert False

[Ipuch]

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

---

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

Previously, pariterre (Pariterre) wrote…

add a comment to explain why even though this is not a fundamental reason why external_forces necessitate ONE_PER_NODE, we made it anyway

Done.

---

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

Previously, pariterre (Pariterre) wrote…

nlp.model.constrained_forward_dynamics

A todo should be added to pass f_ext in controls?

Done

[Ipuch]

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

---

tests/shard1/test_dynamics.py line 42 at r3 (raw file):

Previously, pariterre (Pariterre) wrote…

Reinsert False

Done

[Ipuch]

all green @pariterre

[pariterre]

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

[pariterre]

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