-
Notifications
You must be signed in to change notification settings - Fork 56
[RTM?] Bug fix in holonomic torque dynamics #1021
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
Conversation
Modified the corresponding example so it uses collocation and actually covers the code Also commented out a lot of the compute_all_states function which was unrealiable and not used in the example. We should discuss what to do with this
Ipuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modify the related test that is failing
add the decorator @pytest.parametrize("ode_solver", [OdeSolver.Rk4, OdeSolver.COLLOCATION])
add a if for the collocation values.
@Ipuch reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @p-shg)
bioptim/examples/holonomic_constraints/two_pendulums.py line 84 at r1 (raw file):
# ) return q # , qdot, qddot, lambdas
Decomment
Code quote:
# qdot[:, i] = bio_model.compute_qdot()(q[:, i], states["qdot_u"][:, i]).toarray().squeeze()
# qddot_u_i = (
# bio_model.partitioned_forward_dynamics()(
# states["q_u"][:, i], states["qdot_u"][:, i], q_v_init[:, i], tau[:, i]
# )
# .toarray()
# .squeeze()
# )
# qddot[:, i] = bio_model.compute_qddot()(q[:, i], qdot[:, i], qddot_u_i).toarray().squeeze()
# lambdas[:, i] = (
# bio_model.compute_the_lagrangian_multipliers()(
# states["q_u"][:, i][:, np.newaxis], states["qdot_u"][:, i], q_v_init[:, i], tau[:, i]
# )
# .toarray()
# .squeeze()
# )
return q # , qdot, qddot, lambdasbioptim/examples/holonomic_constraints/two_pendulums.py line 91 at r1 (raw file):
n_shooting: int = 30, final_time: float = 1, expand_dynamics: bool = False,
add ode_solvers: odeSolver = OdeSolver.RK4,
Code quote:
expand_dynamics: bool = False,bioptim/examples/holonomic_constraints/two_pendulums.py line 140 at r1 (raw file):
# Dynamics dynamics = DynamicsOptionsList() dynamics.add(DynamicsOptions(ode_solver=OdeSolver.COLLOCATION(), expand_dynamics=expand_dynamics))
RK4
|
All comments should have been taken into account @Ipuch @pariterre |
pariterre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pariterre reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @p-shg)
|
@p-shg please merge with master and resolve the conflict, then its RTM for me :) |
Specifically using more objects, or more than one constraint, on different objects or on the same object pair. This should help newer users of holonomic constraints understand this better
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1021 +/- ##
==========================================
+ Coverage 77.26% 77.28% +0.02%
==========================================
Files 184 184
Lines 20339 20339
==========================================
+ Hits 15714 15720 +6
+ Misses 4625 4619 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think we're good |
pariterre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pariterre reviewed 13 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @p-shg)
|
@p-shg |
|
Tests should be ok on my side |
|
handled in #1024 |
All Submissions:
New Feature Submissions:
black . -l120 --exclude "external/*")?Changes to Core Features:
This fixes a bug in the dynamics of Holonomic Torque Driven
Bug was not covered by tests since example uses RK4 not COLLOCATION
As such, changed the example to collocation
Also commented out a lot of the compute_all_states because it has been unreliable lately and we only need q in this example. This last point should be discussed
@Ipuch
This change is