Structure refactor #58

Kev1CO · 2024-03-23T01:56:05Z

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 ongoing changes, and removed it when the pull request was ready?
When ready to merge, have you sent a comment pinging @Kev1CO 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

codecov · 2024-03-23T02:18:21Z

Codecov Report

Attention: Patch coverage is 86.48649% with 95 lines in your changes are missing coverage. Please review.

Project coverage is 87.66%. Comparing base (6216856) to head (5c7b763).
Report is 1 commits behind head on main.

Files	Patch %	Lines
cocofest/identification/identification_method.py	50.00%	46 Missing ⚠️
cocofest/integration/ivp_fes.py	90.58%	16 Missing ⚠️
cocofest/optimization/fes_ocp_dynamics.py	82.97%	8 Missing ⚠️
...ication/ding2003_force_parameter_identification.py	82.35%	6 Missing ⚠️
...ication/ding2007_force_parameter_identification.py	78.57%	6 Missing ⚠️
...ication/hmed2018_force_parameter_identification.py	79.31%	6 Missing ⚠️
cocofest/optimization/fes_ocp.py	95.12%	4 Missing ⚠️
cocofest/models/ding2007_with_fatigue.py	85.71%	1 Missing ⚠️
cocofest/models/dynamical_model.py	95.45%	1 Missing ⚠️
cocofest/models/hmed2018_with_fatigue.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   86.44%   87.66%   +1.21%     
==========================================
  Files          18       23       +5     
  Lines        1970     2091     +121     
==========================================
+ Hits         1703     1833     +130     
+ Misses        267      258       -9

Flag	Coverage Δ
unittests	`87.66% <86.48%> (+1.21%)`	⬆️

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.

Ipuch

Minor comments to help you drive safe :)

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

cocofest/models/ding2003.py line 38 at r1 (raw file):

        sum_stim_truncation: int = None,
    ):
        super(FesModel, self).__init__()

Not needed if it is a full abstract class :)

cocofest/models/dynamical_model.py line 149 at r1 (raw file):

            muscle_force_length_coeff = (
                HillCoefficient.muscle_force_length_coefficient(

can't you import the muscle_force_length_coefficient alone ?

cocofest/models/fes_model.py line 8 at r1 (raw file):

class FesModel(ABC):
    def __init__(self):

init is not needed in abstract class :)

cocofest/optimization/fes_ocp.py line 246 at r1 (raw file):

    ):
        if not isinstance(model, FesModel):
            raise TypeError("model must be a FesModel type")

It's fine to let the user know which models

cocofest/optimization/hill_coefficients.py line 4 at r1 (raw file):

class HillCoefficient:

I think I would not have put them in a class. But this is minor

Kev1CO

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

cocofest/models/ding2003.py line 38 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Not needed if it is a full abstract class :)

super().init()
Otherwise pycharm complains
Done.

cocofest/models/dynamical_model.py line 149 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

can't you import the muscle_force_length_coefficient alone ?

Done.

cocofest/models/fes_model.py line 8 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

init is not needed in abstract class :)

Done.

cocofest/optimization/fes_ocp.py line 246 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

It's fine to let the user know which models

Done.

cocofest/optimization/hill_coefficients.py line 4 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I think I would not have put them in a class. But this is minor

Done.

The pulse duration model had an error, it used the frequency model km value instead of his. So after correction, all the asserted values from this new version is changing.

Tested values were changed for ivp_doublet and ivp_triplet as the previous test did not changed the dt (step computation) accordingly to the pulse mode. Therefore the current values are the one expected from correction.

Kev1CO · 2024-04-22T17:41:59Z

cocofest/optimization/fes_ocp.py line 57 at r3 (raw file):

        n_shooting: int = None,
        final_time: int | float = None,
        pulse_apparition_dict: dict = None,

There is many file to revise but they are the concequances of this one. Please @Ipuch can you make your review on this. The other function/examples are using the same dictionnary structure.

Kev1CO · 2024-04-22T17:42:53Z

@Ipuch I pointed the file to revise in reviewable

Ipuch

Reviewed 10 of 32 files at r2, 43 of 43 files at r3, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @Kev1CO)

cocofest/identification/ding2003_force_parameter_identification.py line 40 at r3 (raw file):

    ):
        """
        Attributes

Attributes is for the class documentation. It documents everything is self.XXX. I would expect parameters here in Numpy doc

It looks like this in numpy style.

class NyanCat:
"""
The cat

Attributes
---------------

-
-
-


Methods
------------
- 
-
-
"""

def init(...):
"""
Parameters
---------------
- 
-
"""

cocofest/identification/ding2007_force_parameter_identification.py line 41 at r3 (raw file):

        Initializes the DingModelPulseDurationFrequencyForceParameterIdentification class.

        Parameters

yes

cocofest/identification/ding2007_force_parameter_identification.py line 264 at r3 (raw file):

        return initial_guess

    def force_model_identification(self):

dict[str, np.ndarray]

def force_model_identification(self) -> dict[str, np.ndarray]:

cocofest/identification/identification_method.py line 1 at r3 (raw file):

import pickle

The title of the script is vague
put at least one line docstring under each function to tell what is does, with hints :)

cocofest/integration/ivp_fes.py line 303 at r3 (raw file):

    def _pulse_mode_settings(self):
        if self.pulse_mode == "Single":

lower_case at least if not an enum

cocofest/integration/ivp_fes.py line 491 at r3 (raw file):

        fes_parameters: dict
           The parameters for the fes configuration including :
           model, n_stim, pulse_duration, pulse_intensity, pulse_mode

frequency ?

cocofest/models/hmed2018.py line 341 at r3 (raw file):

        Returns
        -------
        The minimum pulse intensity threshold of the model

maybe a ref/ source here

cocofest/optimization/fes_ocp.py line 57 at r3 (raw file):

Previously, Kev1CO wrote…

There is many file to revise but they are the concequances of this one. Please @Ipuch can you make your review on this. The other function/examples are using the same dictionnary structure.

I'm still not a fan of "pulse apparition" as a term to define timings:

pulse_events_timing ? Ask chagpt4 for ten alternatives

cocofest/optimization/fes_ocp.py line 682 at r3 (raw file):

                expand_continuity=False,
                phase=i,
                # phase_dynamics=PhaseDynamics.ONE_PER_NODE,

deadcode

tests/shard1/test_models_dynamics_without_bioptim.py line 115 at r3 (raw file):

    np.testing.assert_almost_equal(model.cn_dot_fun(cn=0, r0=1.05, t=0.11, t_stim_prev=[0, 0.1]), 36.626599229601936)
    np.testing.assert_almost_equal(model.f_dot_fun(cn=5, f=100, a=3009, tau1=0.050957, km=0.103), 1022.8492662547173)
    np.testing.assert_almost_equal(model.a_dot_fun(a=4900, f=100), 0.1570803149606299)

Are okay about changing core values ?

tests/shard1/test_models_dynamics_without_bioptim.py line 117 at r3 (raw file):

    np.testing.assert_almost_equal(model.a_dot_fun(a=4900, f=100), 0.1570803149606299)
    np.testing.assert_almost_equal(model.tau1_dot_fun(tau1=0.060601, f=100), 0.021)
    np.testing.assert_almost_equal(model.km_dot_fun(km=0.103, f=100), 0.000286716535433071)

idem

tests/shard2/test_fes_dynamics.py line 71 at r3 (raw file):

                0.0001394,
                0.00015499,
                0.00027584,

same

tests/shard2/test_fes_dynamics.py line 88 at r3 (raw file):

                0.00013775,
                0.000141,
                0.0001786,

same

tests/shard2/test_fes_dynamics.py line 99 at r3 (raw file):

    np.testing.assert_almost_equal(sol_states["q"][1][-1], 2.0933333333333333)
    np.testing.assert_almost_equal(sol_states["F_BIClong"][0][-1], 25.871305635093197, decimal=4)
    np.testing.assert_almost_equal(sol_states["F_TRIlong"][0][-1], 11.572282303524243, decimal=4)

why did it changes ?

cocofest/examples/dynamics/frequency_optimization_musculoskeletal_dynamic_2dof.py line 30 at r3 (raw file):

    n_shooting=10,
    final_time=1,
    pulse_apparition_dict={"time_min": 0.01, "time_max": 0.1, "time_bimapping": True},

if the hints are well written, you should not have to specify in the name this is a dict.
Plus, Why not just considering this

 time=dict(min=...,max=...,bimapping=...)

cocofest/examples/dynamics/frequency_optimization_musculoskeletal_dynamic_2dof.py line 31 at r3 (raw file):

    final_time=1,
    pulse_apparition_dict={"time_min": 0.01, "time_max": 0.1, "time_bimapping": True},
    objective_dict={"custom_objective": objective_functions},

same comment for dict

cocofest/examples/dynamics/intensity_optimization_cycling_multi_muscle.py line 51 at r3 (raw file):

    n_shooting=5,
    final_time=1,
    pulse_apparition_dict={"time_min": 0.05, "time_max": 1, "time_bimapping": True},

same

cocofest/examples/dynamics/intensity_optimization_cycling_multi_muscle.py line 52 at r3 (raw file):

    final_time=1,
    pulse_apparition_dict={"time_min": 0.05, "time_max": 1, "time_bimapping": True},
    pulse_intensity_dict={

same

cocofest/examples/getting_started/force_tracking_parameter_optimization.py line 28 at r3 (raw file):

ocp = OcpFes().prepare_ocp(
    model=DingModelIntensityFrequency(),

make sure this is the same Model

model=DingModelIntensityFrequency()
min_pulse_intensity= model.min_pulse_intensity()

ocp = OcpFes().prepare_ocp(
    model=model,
    n_stim=10,
    n_shooting=20,
    final_time=1,
    pulse_intensity_dict={
        "pulse_intensity_min":min_pulse_intensity,
        "pulse_intensity_max": 130,
        "pulse_intensity_bimapping": False,
    },
    objective_dict={"force_tracking": force_tracking},
    use_sx=True,
)

Code quote:

minimum_pulse_intensity = DingModelIntensityFrequency().min_pulse_intensity()

ocp = OcpFes().prepare_ocp(
    model=DingModelIntensityFrequency(),

cocofest/examples/getting_started/muscle_model_identification.py line 327 at r3 (raw file):

pulse_intensity_values = [20, 20, 30, 40, 50, 60, 70, 80, 90, 100]
fes_parameters = {"model": DingModelIntensityFrequency(), "n_stim": 10, "pulse_intensity": pulse_intensity_values}
ivp_parameters = {"n_shooting": 10, "final_time": 1, "use_sx": True}

n_shooting and n_stim look redundant ! This is very obvious

cocofest/examples/getting_started/pulse_duration_optimization.py line 19 at r3 (raw file):

    n_shooting=20,
    final_time=1,
    pulse_apparition_dict={"time_min": 0.01, "time_max": 0.1, "time_bimapping": True},

Same comment dict and its content

cocofest/examples/getting_started/pulse_mode_example.py line 38 at r3 (raw file):

n_stim = 20
final_time = 1
fes_parameters = {"model": DingModelFrequencyWithFatigue(), "n_stim": n_stim, "pulse_mode": "Doublet"}

Lower case Doublet, Triplet etc ...

cocofest/examples/identification/ding2003_model_identification.py line 31 at r3 (raw file):

    "n_shooting": n_shooting,
    "final_time": final_time,
    "extend_last_phase": extra_phase_time,

extend_last_phase, looks like a boolean, I prefer extra_last_phase_time I think :)

Kev1CO · 2024-04-22T19:52:46Z

tests/shard1/test_models_dynamics_without_bioptim.py line 115 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Are okay about changing core values ?

Values changed because a bug where ding2007 was using ding2003 A_rest value instead of his.
Code was corrected and values changed for the correct ones

Kev1CO · 2024-04-22T19:54:12Z

tests/shard2/test_fes_dynamics.py line 71 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same

Same reason as above

Kev1CO

Reviewed 43 of 69 files at r1, 32 of 32 files at r2.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @Ipuch)

cocofest/identification/ding2003_force_parameter_identification.py line 40 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Attributes is for the class documentation. It documents everything is self.XXX. I would expect parameters here in Numpy doc

It looks like this in numpy style.
class NyanCat:
"""
The cat

Attributes
---------------

-
-
-


Methods
------------
- 
-
-
"""

def init(...):
"""
Parameters
---------------
- 
-
"""

Done.

cocofest/identification/ding2007_force_parameter_identification.py line 41 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

yes

Done.

cocofest/identification/ding2007_force_parameter_identification.py line 264 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

dict[str, np.ndarray]

def force_model_identification(self) -> dict[str, np.ndarray]:

Done.

cocofest/identification/identification_method.py line 1 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

The title of the script is vague
put at least one line docstring under each function to tell what is does, with hints :)

Done.

cocofest/integration/ivp_fes.py line 303 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

lower_case at least if not an enum

Done.

cocofest/integration/ivp_fes.py line 491 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

frequency ?

Done.

cocofest/models/hmed2018.py line 341 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

maybe a ref/ source here

Done.

cocofest/optimization/fes_ocp.py line 57 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I'm still not a fan of "pulse apparition" as a term to define timings:

pulse_events_timing ? Ask chagpt4 for ten alternatives

pulse_event
Done.

cocofest/optimization/fes_ocp.py line 682 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

deadcode

Done.

tests/shard1/test_models_dynamics_without_bioptim.py line 117 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

idem

Done.

tests/shard2/test_fes_dynamics.py line 88 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same

Done.

tests/shard2/test_fes_dynamics.py line 99 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

why did it changes ?

Done.

cocofest/examples/dynamics/frequency_optimization_musculoskeletal_dynamic_2dof.py line 30 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

if the hints are well written, you should not have to specify in the name this is a dict.
Plus, Why not just considering this
 time=dict(min=...,max=...,bimapping=...)

Done.

cocofest/examples/dynamics/frequency_optimization_musculoskeletal_dynamic_2dof.py line 31 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same comment for dict

Done.

cocofest/examples/dynamics/intensity_optimization_cycling_multi_muscle.py line 51 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same

Done.

cocofest/examples/dynamics/intensity_optimization_cycling_multi_muscle.py line 52 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same

Done.

cocofest/examples/getting_started/force_tracking_parameter_optimization.py line 28 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

make sure this is the same Model

model=DingModelIntensityFrequency()
min_pulse_intensity= model.min_pulse_intensity()

ocp = OcpFes().prepare_ocp(
    model=model,
    n_stim=10,
    n_shooting=20,
    final_time=1,
    pulse_intensity_dict={
        "pulse_intensity_min":min_pulse_intensity,
        "pulse_intensity_max": 130,
        "pulse_intensity_bimapping": False,
    },
    objective_dict={"force_tracking": force_tracking},
    use_sx=True,
)

Done.

cocofest/examples/getting_started/muscle_model_identification.py line 327 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

n_shooting and n_stim look redundant ! This is very obvious

Done.

cocofest/examples/getting_started/pulse_duration_optimization.py line 19 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Same comment dict and its content

Done.

cocofest/examples/getting_started/pulse_mode_example.py line 38 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Lower case Doublet, Triplet etc ...

Done.

cocofest/examples/identification/ding2003_model_identification.py line 31 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

extend_last_phase, looks like a boolean, I prefer extra_last_phase_time I think :)

Done.

Kev1CO

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

applying refactor advices from pr review

f1fd597

Ipuch reviewed Mar 25, 2024

View reviewed changes

Answered to the first commit comments

8dc92ab

Kev1CO commented Mar 27, 2024

View reviewed changes

Kevin CO added 15 commits March 27, 2024 18:25

Starting identification refactoring

49db93f

refactor on identification

56797b5

Changing assert values

3a6f076

The pulse duration model had an error, it used the frequency model km value instead of his. So after correction, all the asserted values from this new version is changing.

removing mamba info for github action

fbb853d

Ivp refactor

05e1ec9

Tested values were changed for ivp_doublet and ivp_triplet as the previous test did not changed the dt (step computation) accordingly to the pulse mode. Therefore the current values are the one expected from correction.

Documentation and input into dictionaries to improve readability

3d24abd

Modifying entry format for ivp_fes to dictionnaries

3e31ce1

Modifying entry format for ocp fes to dictionnaries

975cc15

Modifying entry format for ocp fes msk to dictionnaries

d483a03

doc string and minor updates

3722336

Applied reviewable comments and doc string

029cedf

fixed a test

d5aee78

black

e3b9ada

new black ver

20afd66

new black ver

b461eac

Ipuch reviewed Apr 22, 2024

View reviewed changes

Kev1CO commented Apr 22, 2024

View reviewed changes

Kevin CO added 2 commits April 22, 2024 17:27

Addressing all reviewable comments

34e4f4a

fixing last typo error

5c7b763

Kev1CO commented Apr 23, 2024

View reviewed changes

Kev1CO merged commit 85fe804 into main Apr 23, 2024
6 of 8 checks passed

Kev1CO deleted the refactoring_ocp_creation branch April 23, 2024 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structure refactor #58

Structure refactor #58

Kev1CO commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading

Ipuch left a comment •

edited

Loading

Kev1CO left a comment

Kev1CO commented Apr 22, 2024

Kev1CO commented Apr 22, 2024

Ipuch left a comment

Kev1CO commented Apr 22, 2024

Kev1CO commented Apr 22, 2024

Kev1CO left a comment

Kev1CO left a comment

Structure refactor #58

Structure refactor #58

Conversation

Kev1CO commented Mar 23, 2024 • edited Loading

All Submissions:

New Feature Submissions:

Changes to Core Features:

codecov bot commented Mar 23, 2024 • edited Loading

Codecov Report

Ipuch left a comment • edited Loading

Choose a reason for hiding this comment

Kev1CO left a comment

Choose a reason for hiding this comment

Kev1CO commented Apr 22, 2024

Kev1CO commented Apr 22, 2024

Ipuch left a comment

Choose a reason for hiding this comment

Kev1CO commented Apr 22, 2024

Kev1CO commented Apr 22, 2024

Kev1CO left a comment

Choose a reason for hiding this comment

Kev1CO left a comment

Choose a reason for hiding this comment

Kev1CO commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading

Ipuch left a comment •

edited

Loading