Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Walking penalties #921

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

EveCharbie
Copy link
Collaborator

@EveCharbie EveCharbie commented Feb 18, 2025

  • Added TRACK_GRF and TRACK_COP penalties (+ tested values)
  • Added a test for the configure variables
  • Did not add an example for the new penalties
  • Did not add the forces to the ExternalForceSetTimeSeries (which should be done in a future refactor, but it is not fast and simple)

This change is Reviewable

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 92.94118% with 6 lines in your changes missing coverage. Please review.

Project coverage is 77.88%. Comparing base (c194ef1) to head (5713f0f).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
bioptim/limits/penalty.py 88.88% 4 Missing ⚠️
bioptim/models/biorbd/biorbd_model.py 95.83% 1 Missing ⚠️
bioptim/optimization/optimization_variable.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
+ Coverage   77.70%   77.88%   +0.18%     
==========================================
  Files         156      156              
  Lines       18011    18074      +63     
==========================================
+ Hits        13995    14077      +82     
+ Misses       4016     3997      -19     
Flag Coverage Δ
unittests 77.88% <92.94%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EveCharbie EveCharbie changed the title [WIP] Walking penalties [RTR] Walking penalties Feb 18, 2025
@EveCharbie
Copy link
Collaborator Author

@Ipuch this too ?

Copy link
Collaborator

@Ipuch Ipuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @EveCharbie)


bioptim/models/biorbd/biorbd_model.py line 615 at r2 (raw file):

        return casadi_fun

    def ground_reaction_forces_and_positions(self, with_position, associated_marker_index: list = None) -> Function:

two separated functions ?


bioptim/models/biorbd/biorbd_model.py line 626 at r2 (raw file):

        tau_biorbd = GeneralizedTorque(self.tau)
        if self.external_force_set is None:
            contact_forces = self.model.ContactForcesFromForwardDynamicsConstraintsDirect(

try to use the method if is exist or create it.


bioptim/models/biorbd/biorbd_model.py line 631 at r2 (raw file):

        else:
            contact_forces = self.model.ContactForcesFromForwardDynamicsConstraintsDirect(
                q_biorbd, qdot_biorbd, tau_biorbd, self.biorbd_external_forces_set

Try to write a one-liner.
biorbd_external_forces_set = self.biorbd_external_forces_set if is not none else None


tests/shard4/test_penalty.py line 104 at r2 (raw file):

    if with_external_forces:
        if not with_contact:
            raise NotImplementedError("External forces must be used with contact")

This case make no sens to test.


bioptim/optimization/optimization_variable.py line 538 at r2 (raw file):

    def shape(self):
        if isinstance(self._unscaled, list) and len(self._unscaled) == 0:
            return 0

remove or raise that says it's empty


bioptim/dynamics/configure_problem.py line 1806 at r2 (raw file):

    @staticmethod
    def configure_contact_forces(ocp, nlp, as_states: bool, as_controls: bool, n_contacts: int = 1):

Translational ? More generic ?


bioptim/limits/penalty.py line 770 at r2 (raw file):

        @staticmethod
        def minimize_ground_reaction_forces(

total_contact_force ?
total_reaction_force ?


bioptim/limits/penalty.py line 771 at r2 (raw file):

        @staticmethod
        def minimize_ground_reaction_forces(
            penalty: PenaltyOption, controller: PenaltyController, contact_index: tuple | list | int | str = None

enlever int et str


bioptim/limits/penalty.py line 774 at r2 (raw file):

        ):
            """
            Simulate force plate data from the contact forces computed through the dynamics with contact

In the case of gait analysis, ...


bioptim/limits/penalty.py line 816 at r2 (raw file):

            controller: PenaltyController,
            associated_marker_index: tuple | list | int | str,
            contact_index: tuple | list | int | str = None,

remove int and str


bioptim/limits/penalty.py line 843 at r2 (raw file):

            penalty.quadratic = True if penalty.quadratic is None else penalty.quadratic

            if controller.external_forces.shape[0] == 0:

the "if case" doesn't make a consistent call of ground_reaction_forces_and_positions


bioptim/limits/penalty.py line 844 at r2 (raw file):

            if controller.external_forces.shape[0] == 0:
                forces_on_each_point, position_of_each_point = controller.model.ground_reaction_forces_and_positions(

C'est nouveau

Code quote:

ground_reaction_forces_and_positions

bioptim/limits/penalty.py line 862 at r2 (raw file):

            for i_component in range(3):
                center_of_pressure[i_component] = if_else(
                    total_force[i_component] < 1e-6, 0, val[i_component] / total_force[i_component]

barycenter = val[i_component] / total_force[i_component]
separate the physics from the numerical hack.

@Ipuch Ipuch changed the title [RTR] Walking penalties [WIP] Walking penalties Feb 19, 2025
Copy link
Collaborator Author

@EveCharbie EveCharbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


bioptim/dynamics/configure_problem.py line 1806 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Translational ? More generic ?

I put translational for now, and will open a new PR to include the forces as a symboli_external_force_set


bioptim/limits/penalty.py line 770 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

total_contact_force ?
total_reaction_force ?

Done.


bioptim/limits/penalty.py line 771 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

enlever int et str

Done.


bioptim/limits/penalty.py line 774 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

In the case of gait analysis, ...

Done.


bioptim/limits/penalty.py line 816 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

remove int and str

Done.


bioptim/limits/penalty.py line 843 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

the "if case" doesn't make a consistent call of ground_reaction_forces_and_positions

Done.


bioptim/limits/penalty.py line 844 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

C'est nouveau

Done.


bioptim/limits/penalty.py line 862 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

barycenter = val[i_component] / total_force[i_component]
separate the physics from the numerical hack.

Done.


bioptim/models/biorbd/biorbd_model.py line 615 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

two separated functions ?

Done.


bioptim/models/biorbd/biorbd_model.py line 626 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

try to use the method if is exist or create it.

Done.


bioptim/models/biorbd/biorbd_model.py line 631 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Try to write a one-liner.
biorbd_external_forces_set = self.biorbd_external_forces_set if is not none else None

Done.


bioptim/optimization/optimization_variable.py line 538 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

remove or raise that says it's empty

Done.


tests/shard4/test_penalty.py line 104 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

This case make no sens to test.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants