[RTR] Multi Biorbd Model #625

EveCharbie · 2023-02-23T15:54:19Z

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:

Does your submission pass the tests (if not please explain why this is intended)?
Did you write a proper documentation (docstrings and ReadMe)
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 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EveCharbie)

bioptim/interfaces/biomodel.py line 17 at r1 (raw file):

    """Get the gravity vector"""

    def set_gravity(self, new_gravity, model_index: int = 0):

Change those for not implemented when required

Code quote:

y, model_index: int = 0

bioptim/interfaces/biorbd_model.py line 4 at r1 (raw file):

from ..misc.utils import check_version
from .multi_biorbd_model import MultiBiorbdModel

Keep them in a single file

Code quote:

from ..misc.utils import check_version
from .multi_biorbd_model import MultiBiorbdModel

EveCharbie

Reviewable status: 1 of 14 files reviewed, 2 unresolved discussions (waiting on @pariterre)

bioptim/interfaces/biomodel.py line 17 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Change those for not implemented when required

Done.

bioptim/interfaces/biorbd_model.py line 4 at r1 (raw file):

Previously, pariterre (Pariterre) wrote…

Keep them in a single file

Done.

codecov · 2023-02-24T20:47:15Z

Codecov Report

Patch coverage: 92.30% and project coverage change: +0.19 🎉

Comparison is base (de14c12) 81.70% compared to head (03c0d49) 81.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
+ Coverage   81.70%   81.90%   +0.19%     
==========================================
  Files         108      109       +1     
  Lines       11924    12133     +209     
==========================================
+ Hits         9743     9937     +194     
- Misses       2181     2196      +15

Impacted Files	Coverage Δ
bioptim/limits/phase_transition.py	`92.22% <50.00%> (ø)`
...es/torque_driven_ocp/example_multi_biorbd_model.py	`72.72% <72.72%> (ø)`
bioptim/interfaces/biorbd_model.py	`96.01% <95.55%> (+1.22%)`	⬆️
bioptim/__init__.py	`100.00% <100.00%> (ø)`
bioptim/dynamics/configure_problem.py	`91.32% <100.00%> (+0.01%)`	⬆️
bioptim/interfaces/biomodel.py	`100.00% <100.00%> (ø)`
bioptim/limits/constraints.py	`83.21% <100.00%> (ø)`
bioptim/optimization/optimal_control_program.py	`87.15% <100.00%> (+0.02%)`	⬆️

... and 2 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

EveCharbie · 2023-02-24T21:12:44Z

@pariterre I think we are good to go :)
Please verify that you agree with which functions I decided to overwritte in BiorbdModel (which for me does not make sens for multiple models)

pariterre

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

bioptim/interfaces/biorbd_model.py line 479 at r2 (raw file):

            raise ValueError("Wrong index")
        # raise NotImplementedError("rigid_contact_acceleration is not implemented yet for multi models") # @pariterre I think we should remove it if we know it is broken!
        return self.model.rigidContactAcceleration(q, qdot, qddot, 0, True).to_mx()[index_direction]

Can we not just change this "0" for "index"? If this fails (for instance Segmentation Fault), I would vote for removing this method altogether

Code quote:

tests/test_biorbd_multi_model.py line 82 at r2 (raw file):

    models.set_gravity(np.array([0, 0, -3]))
    model_gravity_modified = Function("Gravity", [], [models.gravity])()["o0"]
    # segment_index = models.segment_index("Seg1")

This should expect a raise

with pytest.raise(RuntimeError, match="error message"):
    segment_index = models.segment_index("Seg1")

Those below as well!

Code quote:

 # segment_index = models.segment_index("Seg1")

tests/test_global_torque_driven_ocp.py line 583 at r2 (raw file):

    return  # TODO: when it is not broken anymore, the following results should be good

    sol = ocp.solve()

What was the reason to remove those?

Code quote:

    np.testing.assert_almost_equal(g, np.zeros((40, 1)), decimal=6)

    # Check some of the results
    states, controls, states_no_intermediate = sol.states, sol.controls, sol.states_no_intermediate

    # initial and final position
    np.testing.assert_almost_equal(states[0]["q"][:, 0], np.array([-3.14159265, 0.0]), decimal=6)
    np.testing.assert_almost_equal(states[0]["q"][:, -1], np.array([3.04159296, 0.0]), decimal=3)
    np.testing.assert_almost_equal(states[1]["q"][:, 0], np.array([-3.14159265, 0.0]), decimal=6)
    np.testing.assert_almost_equal(states[1]["q"][:, -1], np.array([3.04159271, 0.0]), decimal=6)
    # initial and final velocities
    np.testing.assert_almost_equal(states[0]["qdot"][:, 0], np.array([11.47768245, 26.16790572]), decimal=6)
    np.testing.assert_almost_equal(states[0]["qdot"][:, -1], np.array([11.52232512, 26.06343438]), decimal=6)
    np.testing.assert_almost_equal(states[1]["qdot"][:, 0], np.array([10.54030594, 28.30202101]), decimal=6)
    np.testing.assert_almost_equal(states[1]["qdot"][:, -1], np.array([10.594124, 28.17553337]), decimal=6)
    # initial and final controls
    np.testing.assert_almost_equal(controls[0]["tau"][:, 0], np.array([0.01906557]), decimal=6)
    np.testing.assert_almost_equal(controls[0]["tau"][:, -2], np.array([-0.00619146]), decimal=6)
    np.testing.assert_equal(controls[1], {})

EveCharbie

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

bioptim/interfaces/biorbd_model.py line 479 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Can we not just change this "0" for "index"? If this fails (for instance Segmentation Fault), I would vote for removing this method altogether

Fatal Python error: Segmentation fault
But it is used in the DAE_INVERSE_DYNAMICS with contact.
Do we let it as is ?

tests/test_biorbd_multi_model.py line 82 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

This should expect a raise

with pytest.raise(RuntimeError, match="error message"):
    segment_index = models.segment_index("Seg1")

Those below as well!

Done.

tests/test_global_torque_driven_ocp.py line 583 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

What was the reason to remove those?

Was not tested anyway, but yeah I can put it back.

pariterre

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

bioptim/interfaces/biorbd_model.py line 479 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Fatal Python error: Segmentation fault
But it is used in the DAE_INVERSE_DYNAMICS with contact.
Do we let it as is ?

@Ipuch
Is this expected?

Ipuch

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @pariterre)

bioptim/interfaces/biorbd_model.py line 479 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

@Ipuch
Is this expected?

This is expected as there is only one contact in the model. But yes, we should specify the index instead of 0. And remove the TODO.

Ipuch

Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions (waiting on @EveCharbie and @pariterre)

bioptim/interfaces/biorbd_model.py line 337 at r2 (raw file):

            out = vertcat(
                out,
                model.InverseDynamics(

for i, model in enumerate(self.models):
       q = q[self.models.index(variable: str = "q", model_index: int = i)]
       ...

So don't have to track q, qdot and qddot, Disccussed with benjamin. Variable is expected to receive "q", "qdot", "qddot", "markers", "contact", etc...

Need to be applied everywhere you felt the need of tracking. plus this function should be useful for the objective or constraint definition.

Suggestion:

seDynamics(

pariterre

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

bioptim/interfaces/biorbd_model.py line 337 at r2 (raw file):

self.models.index(variable: str = "q", model_index: int = i)

self.variable_index(variable: str, model_index: int) -> slice

for instance: return slice(init_index, final_index)

EveCharbie

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Ipuch and @pariterre)

bioptim/interfaces/biorbd_model.py line 337 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

self.models.index(variable: str = "q", model_index: int = i)

self.variable_index(variable: str, model_index: int) -> slice

for instance: return slice(init_index, final_index)

Done.

bioptim/interfaces/biorbd_model.py line 479 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

This is expected as there is only one contact in the model. But yes, we should specify the index instead of 0. And remove the TODO.

Done.

EveCharbie · 2023-03-07T22:09:24Z

@pariterre I think it is ready to merge :)

pariterre

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

Ipuch

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)

EveCharbie added 13 commits February 16, 2023 16:27

First commit from pariterre

37d14a9

Added example

5fe54da

Cleaned example

1000e4d

added missing functions

1767127

started testing

cc21733

added all the tests

4049ff0

Readded the models for the example

c322976

fixed some functions with right shape Q

f146939

finished the first version of the tests

b9bfa7b

fixed the tests for multi_biorbd_model

a90ffc1

added default model number to 0 for retro compatibility

5fd95cb

BiorbdModel now heritates from MultiBiorbdModel

d0b1439

blacked

7122fbf

pariterre requested changes Feb 23, 2023

View reviewed changes

EveCharbie added 12 commits February 23, 2023 15:04

fixed dynamics problems

c51734d

fixed +cleaned example

048f4dd

changed bound on biomod for the example

3050e9d

moved stuff + removed model_index

b6d40ab

small modifs

a0926d6

made some tests pass

c829064

small fix

c45d8b0

doc

608d87b

fixed soft contact test

9c6f693

added tests

31d7ef2

blacked

16a55cf

Tries to fix tests to pass remotely ?!

0f94900

EveCharbie commented Feb 24, 2023

View reviewed changes

EveCharbie added 3 commits February 24, 2023 09:53

blacked

8015470

Fixed tests ?

f74904a

blacked

872d60e

EveCharbie added 3 commits February 24, 2023 13:25

argh!

c7b6915

reverted unintended changes

a4eb419

got mixed up, reverted

ef5c335

EveCharbie changed the title ~~[WIP] Multi Biorbd Model~~ [RTR] Multi Biorbd Model Feb 24, 2023

pariterre requested changes Feb 24, 2023

View reviewed changes

EveCharbie commented Feb 25, 2023

View reviewed changes

Made changes requested by pariterre

cb42e2f

pariterre requested changes Feb 27, 2023

View reviewed changes

EveCharbie added 2 commits March 2, 2023 09:08

fixed last test hopefully

8d2e136

blacked

e31ecac

Ipuch reviewed Mar 2, 2023

View reviewed changes

Ipuch requested changes Mar 2, 2023

View reviewed changes

pariterre reviewed Mar 2, 2023

View reviewed changes

added a method for variable index

9e8a08b

EveCharbie commented Mar 2, 2023

View reviewed changes

EveCharbie added 5 commits March 2, 2023 18:46

blacked

3ab325e

Fixed contacts with Pierre's comment

b1f9052

Merge remote-tracking branch 'pyomeca/master' into multi_biorbd_model

7f95273

blacked

6bb5495

corrected merge fuck up

03c0d49

pariterre approved these changes Mar 9, 2023

View reviewed changes

Ipuch approved these changes Mar 9, 2023

View reviewed changes

pariterre merged commit 3013567 into pyomeca:master Mar 9, 2023

EveCharbie deleted the multi_biorbd_model branch May 30, 2023 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTR] Multi Biorbd Model #625

[RTR] Multi Biorbd Model #625

EveCharbie commented Feb 23, 2023 •

edited

Loading

pariterre left a comment

EveCharbie left a comment

codecov bot commented Feb 24, 2023 •

edited

Loading

EveCharbie commented Feb 24, 2023

pariterre left a comment

EveCharbie left a comment

pariterre left a comment

Ipuch left a comment

Ipuch left a comment

pariterre left a comment

EveCharbie left a comment

EveCharbie commented Mar 7, 2023

pariterre left a comment

Ipuch left a comment

[RTR] Multi Biorbd Model #625

[RTR] Multi Biorbd Model #625

Conversation

EveCharbie commented Feb 23, 2023 • edited Loading

All Submissions:

New Feature Submissions:

Changes to Core Features:

pariterre left a comment

Choose a reason for hiding this comment

EveCharbie left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

EveCharbie commented Feb 24, 2023

pariterre left a comment

Choose a reason for hiding this comment

EveCharbie left a comment

Choose a reason for hiding this comment

pariterre left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

pariterre left a comment

Choose a reason for hiding this comment

EveCharbie left a comment

Choose a reason for hiding this comment

EveCharbie commented Mar 7, 2023

pariterre left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

EveCharbie commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading