Migrate cocofest to bioptim 3.4#27
Conversation
mickaelbegon
force-pushed
the
pyomeca-main/bioptim-3-4-migration
branch
from
6b64b68 to
bb5357f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 67.52% 68.03% +0.51%
==========================================
Files 38 38
Lines 3002 3194 +192
==========================================
+ Hits 2027 2173 +146
- Misses 975 1021 +46
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:
|
Kev1CO
left a comment
Kev1CO
left a comment
There was a problem hiding this comment.
@Kev1CO reviewed 6 files and all commit messages, and made 8 comments.
Reviewable status: 6 of 34 files reviewed, 8 unresolved discussions (waiting on mickaelbegon).
environment.yml line 9 at r2 (raw file):
# - This branch targets bioptim 3.4.0 from conda-forge. # - Keep `external/bioptim` out of PYTHONPATH so the conda package is used. # - `ma57` is optional. If `libhsl.dylib` is available on your machine, point IPOPT to it.
remove
Code quote:
# Create the branch environment with:
# conda env create -f environment.yml
# or update an existing one with:
# conda env update -f environment.yml --prune
#
# Notes:
# - This branch targets bioptim 3.4.0 from conda-forge.
# - Keep `external/bioptim` out of PYTHONPATH so the conda package is used.
# - `ma57` is optional. If `libhsl.dylib` is available on your machine, point IPOPT to it.cocofest/models/dynamical_model.py line 143 at r2 (raw file):
def muscle_name_dof(self, index: int = 0) -> list[str]: muscle = self.muscles_dynamics_model[index] return [f"{name}_{muscle.muscle_name}" for name in muscle.name_dof]
was not used to print before, if this feature is kept, add print key in function (set to false default)
Code quote:
[f"{name}_{muscle.muscle_name}" for name in muscle.name_dof]cocofest/models/dynamical_model.py line 201 at r2 (raw file):
) return controls
To many nested for/if loops, create new functions to make the code lecture more understandable.
Code quote:
@property
def state_configuration_functions(self):
state_configure = StateConfigure()
muscle_state_configurations = []
for muscle_model in self.muscles_dynamics_model:
for state_key in muscle_model.name_dof:
if state_key in state_configure.state_dictionary:
muscle_state_configurations.append(
lambda ocp, nlp, state_key=state_key, muscle_model=muscle_model: state_configure.state_dictionary[
state_key
](
ocp=ocp,
nlp=nlp,
as_states=True,
as_controls=False,
muscle_name=muscle_model.muscle_name,
)
)
return muscle_state_configurations + [
lambda ocp, nlp: ConfigureVariables.configure_q(ocp, nlp, as_states=True),
lambda ocp, nlp: ConfigureVariables.configure_qdot(ocp, nlp, as_states=True),
]
@property
def control_configuration_functions(self):
controls = []
if self.activate_residual_torque:
controls.append(lambda ocp, nlp: ConfigureVariables.configure_tau(ocp, nlp, as_controls=True))
for muscle_model in self.muscles_dynamics_model:
if isinstance(muscle_model, DingModelPulseWidthFrequency):
controls.append(
lambda ocp, nlp, muscle_model=muscle_model: StateConfigure().configure_last_pulse_width(
ocp, nlp, muscle_model.muscle_name
)
)
if isinstance(muscle_model, DingModelPulseIntensityFrequency):
controls.append(
lambda ocp, nlp, muscle_model=muscle_model: StateConfigure().configure_pulse_intensity(
ocp, nlp, muscle_model.muscle_name, muscle_model.sum_stim_truncation
)
)
return controlscocofest/models/state_configure.py line 491 at r2 (raw file):
state_name_list.append(state_key + "_" + muscle_dynamics_model.muscle_name) return state_name_list
Similarly, to many for/if loops, refactor
Code quote:
def configure_all_muscle_states(self, muscles_dynamics_model, ocp, nlp):
state_name_list = []
for muscle_dynamics_model in muscles_dynamics_model:
for state_key in muscle_dynamics_model.name_dof:
if state_key in self.state_dictionary.keys():
self.state_dictionary[state_key](
ocp=ocp,
nlp=nlp,
as_states=True,
as_controls=False,
muscle_name=muscle_dynamics_model.muscle_name,
)
state_name_list.append(state_key + "_" + muscle_dynamics_model.muscle_name)
return state_name_listcocofest/models/ding2003/ding2003.py line 396 at r2 (raw file):
controls=controls, numerical_timeseries=numerical_timeseries, )
Why try / except ?
Code quote:
try:
dxdt = dxdt_fun(
cn=states[0],
f=states[1],
t=time,
t_stim_prev=numerical_timeseries,
force_length_relationship=force_length_relationship,
force_velocity_relationship=force_velocity_relationship,
passive_force_relationship=passive_force_relationship,
)
except TypeError:
dxdt = dxdt_fun(
time=time,
states=states,
controls=controls,
numerical_timeseries=numerical_timeseries,
)cocofest/models/ding2003/ding2003_with_fatigue.py line 187 at r2 (raw file):
The value of the derivative of each state dx/dt at the current time t """ if states is not None:
Same ? should'nt it be always not none ?
cocofest/models/ding2003/ding2003_with_fatigue.py line 194 at r2 (raw file):
km = states[4] if time is not None: t = time
Why ? shouldn't time alway be not None ?
Code quote:
if time is not None:
t = timecocofest/models/ding2007/ding2007.py line 160 at r2 (raw file):
f = states[1] if time is not None: t = time
Similar
Code quote:
if states is not None:
cn = states[0]
f = states[1]
if time is not None:
t = time
|
? Code quote: @property
def name_dofs(self) -> list[str]:
return self.name_dof |
Kev1CO
left a comment
Kev1CO
left a comment
There was a problem hiding this comment.
@Kev1CO made 13 comments.
Reviewable status: 6 of 34 files reviewed, 21 unresolved discussions (waiting on mickaelbegon).
cocofest/models/hmed2018/hmed2018_with_fatigue.py line 70 at r2 (raw file):
Previously, Kev1CO (Kevin Co) wrote…
?
what happen here
cocofest/models/ding2003/ding2003_with_fatigue.py line 345 at r2 (raw file):
""" StateConfigure().configure_all_fes_model_states(ocp, nlp, fes_model=self) ConfigureProblem.configure_dynamics_function(ocp, nlp, dyn_func=self.dynamics)
Why does it show as new lines, when this already exist in file
Code quote:
@staticmethod
def dynamics(
time: MX,
states: MX,
controls: MX,
parameters: MX,
algebraic_states: MX,
numerical_timeseries: MX,
nlp: NonLinearProgram,
fes_model=None,
force_length_relationship: MX | float = 1,
force_velocity_relationship: MX | float = 1,
passive_force_relationship: MX | float = 0,
) -> DynamicsEvaluation:
"""
Functional electrical stimulation dynamic
Parameters
----------
time: MX
The system's current node time
states: MX
The state of the system CN, F, A, Tau1, Km
controls: MX
The controls of the system, none
parameters: MX
The parameters acting on the system, final time of each phase
algebraic_states: MX
The stochastic variables of the system, none
numerical_timeseries: MX
The numerical timeseries of the system
nlp: NonLinearProgram
A reference to the phase
fes_model: DingModelFrequencyWithFatigue
The current phase fes model
force_length_relationship: MX | float
The force length relationship value (unitless)
force_velocity_relationship: MX | float
The force velocity relationship value (unitless)
passive_force_relationship: MX | float
The passive force relationship value (unitless)
Returns
-------
The derivative of the states in the tuple[MX] format
"""
model = fes_model if fes_model else nlp.model
dxdt_fun = model.system_dynamics
return DynamicsEvaluation(
dxdt=dxdt_fun(
cn=states[0],
f=states[1],
a=states[2],
tau1=states[3],
km=states[4],
t=time,
t_stim_prev=numerical_timeseries,
force_length_relationship=force_length_relationship,
force_velocity_relationship=force_velocity_relationship,
passive_force_relationship=passive_force_relationship,
),
defects=None,
)
def declare_ding_variables(
self,
ocp: OptimalControlProgram,
nlp: NonLinearProgram,
numerical_data_timeseries: dict[str, np.ndarray] = None,
contact_type: list = (),
):
"""
Tell the program which variables are states and controls.
The user is expected to use the ConfigureProblem.configure_xxx functions.
Parameters
----------
ocp: OptimalControlProgram
A reference to the ocp
nlp: NonLinearProgram
A reference to the phase
numerical_data_timeseries: dict[str, np.ndarray]
A list of values to pass to the dynamics at each node. Experimental external forces should be included here.
contact_type: list
A list of contact types. This is used to define the contact forces in the dynamics. Not used in this model.
"""
StateConfigure().configure_all_fes_model_states(ocp, nlp, fes_model=self)
ConfigureProblem.configure_dynamics_function(ocp, nlp, dyn_func=self.dynamics)cocofest/models/ding2007/ding2007.py line 84 at r2 (raw file):
@property def control_configuration_functions(self): return [lambda ocp, nlp: StateConfigure().configure_last_pulse_width(ocp, nlp, self.muscle_name)]
Why was it moved underneath
Code quote:
@property
def control_configuration_functions(self):
return [lambda ocp, nlp: StateConfigure().configure_last_pulse_width(ocp, nlp, self.muscle_name)]cocofest/models/ding2007/ding2007.py line 331 at r2 (raw file):
states=states, controls=controls, numerical_timeseries=numerical_timeseries,
Okay, I might understand, this is for the non fatigue and fatigue version (parent-child). But if its the case that why is there still the dynamics function in the child class?
Is it because bioptim requires the function dynamics in each models ? If yes than remove the try except
Code quote:
try:
dxdt = dxdt_fun(
cn=states[0],
f=states[1],
t=time,
t_stim_prev=numerical_timeseries,
pulse_width=controls[0],
force_length_relationship=force_length_relationship,
force_velocity_relationship=force_velocity_relationship,
passive_force_relationship=passive_force_relationship,
)
except TypeError:
dxdt = dxdt_fun(
time=time,
states=states,
controls=controls,
numerical_timeseries=numerical_timeseries,cocofest/models/ding2007/ding2007_with_fatigue.py line 184 at r2 (raw file):
km = states[4] if time is not None: t = time
same
Code quote:
if states is not None:
cn = states[0]
f = states[1]
a = states[2]
tau1 = states[3]
km = states[4]
if time is not None:
t = timecocofest/models/hmed2018/hmed2018.py line 89 at r2 (raw file):
ocp, nlp, self.muscle_name, self.sum_stim_truncation ) ]
same, why did it move downwards?
Code quote:
@property
def control_configuration_functions(self):
return [
lambda ocp, nlp: StateConfigure().configure_pulse_intensity(
ocp, nlp, self.muscle_name, self.sum_stim_truncation
)
]examples/fes_multibody/cycling/cost_functions.py line 1 at r2 (raw file):
from casadi import MX, vertcat, sum1, fabs, sign, tanh, if_else, log, exp, DM, dot, mmax, mmin, cos, sin, sqrt
Remove this file
Code quote:
from casadi import Mexamples/fes_multibody/cycling/cycling_bayesian_mhe.py line 2 at r2 (raw file):
""" This example will perform a bayesian optimization on an optimal control program moving time horizon for a hand cycling
This version of cycling is outdated
At the end of all comment in the PR, add the new one
examples/fes_multibody/cycling/cycling_pulse_width_mhe.py line 1482 at r2 (raw file):
# ["minimize_average_fatigue_and_recovery_2"], # ["minimize_balanced_fatigue_by_contribution"], ]
Similar to bayesian example
Code quote:
# --- Recovery --- #
["minimize_average_fatigue_and_recovery"],
# ["minimize_average_fatigue_and_recovery_2"],
# ["minimize_balanced_fatigue_by_contribution"],
]examples/fes_multibody/cycling/cycling_standard_clinics.py line 10 at r2 (raw file):
from sys import platform import pickle
similar
examples/fes_multibody/cycling/cycling_with_different_driven_methods.py line 618 at r2 (raw file):
solver.set_option_unsafe(2.0, "ma57_pre_alloc") solver.set_option_unsafe(1e-6, "acceptable_tol") solver.set_option_unsafe(12, "acceptable_iter")
why
Code quote:
solver.set_option_unsafe(1e-6, "acceptable_tol")
solver.set_option_unsafe(12, "acceptable_iter")examples/fes_multibody/cycling/cycling_with_different_driven_methods.py line 643 at r2 (raw file):
) parser.add_argument("--max-iter", type=int, default=6000, help="Maximum IPOPT iterations.") args = parser.parse_args()
dont use parser
Code quote:
parser = argparse.ArgumentParser(description="Run the cycling OCP with different driven methods.")
parser.add_argument("--plot", action="store_true", help="Display animation and graphs after solving.")
parser.add_argument(
"--objective-mode",
type=str,
default="endurance_1500_weighted_fatigue",
choices=["force_production", "endurance_1500_weighted_fatigue"],
help="Objective used for the FES-driven OCP.",
)
parser.add_argument(
"--linear-solver",
type=str,
default="ma57",
help="IPOPT linear solver to use, e.g. ma57 or mumps.",
)
parser.add_argument("--max-iter", type=int, default=6000, help="Maximum IPOPT iterations.")
args = parser.parse_args()tests/shard1/test_bioptim_3_4_compat.py line 29 at r2 (raw file):
assert ding2007._with_fatigue is True assert ding2003.name_dofs == ["Cn", "F", "A", "Tau1", "Km"] assert ding2007.name_dofs == ["Cn", "F", "A", "Tau1", "Km"]
Overlap with test_models_dynamics_without_bioptim
Code quote:
def test_model_maker_instantiates_ding_models_on_bioptim_3_4():
ding2003 = ModelMaker.create_model("ding2003_with_fatigue", stim_time=[0, 0.1], sum_stim_truncation=10)
ding2007 = ModelMaker.create_model("ding2007_with_fatigue", stim_time=[0, 0.1], sum_stim_truncation=10)
assert ding2003._with_fatigue is True
assert ding2007._with_fatigue is True
assert ding2003.name_dofs == ["Cn", "F", "A", "Tau1", "Km"]
assert ding2007.name_dofs == ["Cn", "F", "A", "Tau1", "Km"]
Kev1CO
left a comment
Kev1CO
left a comment
There was a problem hiding this comment.
@Kev1CO reviewed 1 file and all commit messages, made 13 comments, and resolved 3 discussions.
Reviewable status: 7 of 34 files reviewed, 29 unresolved discussions (waiting on mickaelbegon).
cocofest/models/ding2003/ding2003_with_fatigue.py line 187 at r2 (raw file):
Previously, Kev1CO (Kevin Co) wrote…
Same ? should'nt it be always not none ?
Follow up
cocofest/models/ding2003/ding2003_with_fatigue.py line 194 at r2 (raw file):
Previously, Kev1CO (Kevin Co) wrote…
Why ? shouldn't time alway be not None ?
Same, want an answer
cocofest/models/dynamical_model.py line 216 at r3 (raw file):
for state_key in muscle_model.name_dof if state_key in state_dictionary ]
This list comprehension is way to packed.
Can you separate the muscle state key configuration from the muscle models in the msk model ?
Or can you use the configure_all_muscle_states from state_configure.py
Code quote:
def _muscle_state_configuration_functions(self):
state_configure = StateConfigure()
state_dictionary = state_configure.state_dictionary
return [
(
lambda ocp, nlp, state_key=state_key, muscle_model=muscle_model: state_dictionary[state_key](
ocp=ocp,
nlp=nlp,
as_states=True,
as_controls=False,
muscle_name=muscle_model.muscle_name,
)
)
for muscle_model in self.muscles_dynamics_model
for state_key in muscle_model.name_dof
if state_key in state_dictionary
]cocofest/models/ding2003/ding2003.py line 211 at r3 (raw file):
@staticmethod def _legacy_state_inputs(cn, f, t, t_stim_prev, states, time, numerical_timeseries):
Can you explain to me the purpose of this addition?
cocofest/models/ding2003/ding2003.py line 399 at r3 (raw file):
"tau1": states[3], "km": states[4], }
Why are you adding this here? The fatigue model (child of this class) has its own dynamics function so model.fatigue can't be true here.
Code quote:
if model.with_fatigue:
dynamics_kwargs |= {
"a": states[2],
"tau1": states[3],
"km": states[4],
}cocofest/models/ding2003/ding2003.py line 401 at r3 (raw file):
} if "cn" in inspect.signature(dxdt_fun).parameters: dxdt = dxdt_fun(**dynamics_kwargs)
Can you explain the meaning of this to me.
Code quote:
if "cn" in inspect.signature(dxdt_fun).parameters:
dxdt = dxdt_fun(**dynamics_kwargs)cocofest/models/ding2003/ding2003_with_fatigue.py line 104 at r3 (raw file):
# ---- Needed for the example ---- # @property def name_dof(self, with_muscle_name: bool = False) -> list[str]:
why removed the s?
Code quote:
of(cocofest/models/ding2003/ding2003_with_fatigue.py line 167 at r3 (raw file):
The value of the force (N) a: MX The value of the scaling factor (unitless)
not unitless: s^-2
cocofest/models/ding2007/ding2007.py line 324 at r3 (raw file):
"tau1": states[3], "km": states[4], }
Similar comment than for the ding2003 model
Fatigue can not be here + what is the inspect.signature thing
Code quote:
if model.with_fatigue:
dynamics_kwargs |= {
"a": states[2],
"tau1": states[3],
"km": states[4],
}cocofest/models/ding2007/ding2007_with_fatigue.py line 155 at r3 (raw file):
The value of the force (N) a: MX The value of the scaling factor (unitless)
not unitless
cocofest/models/ding2007/ding2007_with_fatigue.py line 317 at r3 (raw file):
state_names = DingModelPulseWidthFrequency._get_state_dot_names(nlp, model) states_dot = DingModelPulseWidthFrequency._get_states_dot_scaled_cx(nlp, state_names) defects = states_dot * nlp.dt - dxdt * nlp.dt
This is needed for collocation, I used it for this model specifically but can you make it available for the other model (the ding2003 and hmed2018) otherwise they are not collocation ready
Code quote:
defects = None
if isinstance(nlp.dynamics_type.ode_solver, OdeSolver.COLLOCATION) and nlp.model is model:
state_names = DingModelPulseWidthFrequency._get_state_dot_names(nlp, model)
states_dot = DingModelPulseWidthFrequency._get_states_dot_scaled_cx(nlp, state_names)
defects = states_dot * nlp.dt - dxdt * nlp.dtcocofest/models/hmed2018/hmed2018.py line 178 at r3 (raw file):
t_stim_prev = numerical_timeseries if controls is not None: pulse_intensity = controls
Similar, why these?
Code quote:
if states is not None:
cn = states[0]
f = states[1]
if time is not None:
t = time
if numerical_timeseries is not None:
t_stim_prev = numerical_timeseries
if controls is not None:
pulse_intensity = controlscocofest/models/hmed2018/hmed2018_with_fatigue.py line 172 at r3 (raw file):
tau1 = self.tau1_rest if km is None: km = self.km_rest
similar
Code quote:
cn, f, t, t_stim_prev = self._legacy_state_inputs(cn, f, t, t_stim_prev, states, time, numerical_timeseries)
if states is not None:
a = states[2]
tau1 = states[3]
km = states[4]
if controls is not None:
pulse_intensity = controls
if a is None:
a = self.a_rest
if tau1 is None:
tau1 = self.tau1_rest
if km is None:
km = self.km_rest
|
Thanks, I went through the latest review pass and pushed a follow-up in Addressed in code:
A few direct answers to the review questions:
Validation on my side after this push:
I’ll keep an eye on the next review round and fold in any follow-up comments. |
|
Small follow-up on the remaining review questions on this PR: the latest push on this branch is still What changed relative to the older review snapshots:
And to answer the “why is this here?” questions directly:
I re-ran after that cleanup:
If you want, I can also go thread-by-thread in Reviewable wording and answer each point more literally there, but from the code side the follow-up concerns above are now reflected in the branch. |
Kev1CO
left a comment
Kev1CO
left a comment
There was a problem hiding this comment.
@Kev1CO reviewed 27 files and all commit messages, and resolved 21 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on mickaelbegon).
2 participants
Summary
Note
Tests
This change is