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] Update of the inertial parameters #100 #105

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ANaaim
Copy link
Contributor

@ANaaim ANaaim commented Oct 31, 2023

  • Linked issue related to your pull request using 'Refactoring Inertial Parameters & Improvements Needed #100'
  • Used the tag [WIP] or [RTR] for on-going changes, or for ready to review
  • Did you write new tests to cover your changes?
  • Did you write a proper documentation (docstrings and ReadMe)
  • Please black your code black . -l120

This change is Reviewable

Copy link
Owner

@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 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ANaaim)


bionc/model_creation/biomechanical_model_template.py line 129 at r1 (raw file):

                    inertia_matrix=s.inertia_parameters.inertia,
                    transformation_matrix=TransformationMatrixType.Buv,
                )

Okay, does this works for you ? here are my toughts:

The user should only be considered by the choice of Inertia Parameters Antropometric Table Item such as Dumas2007.Thigh for example.
thus, at that stage we need to be able to pass the length computed just before ("v = rp -rd") to compute the inertia parameters from the identified length.

What do you think ?

Also, if we uncomment this feature, we need to test to maintain it 👍

Code quote:

                natural_segment.set_inertial_parameters(
                    mass=s.inertia_parameters.relative_mass,
                    center_of_mass=s.inertia_parameters.center_of_mass,
                    inertia_matrix=s.inertia_parameters.inertia,
                    transformation_matrix=TransformationMatrixType.Buv,
                )

@Ipuch Ipuch added the enhancement New feature or request label Nov 2, 2023
Copy link
Owner

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ANaaim)


bionc/model_creation/biomechanical_model_template.py line 129 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Okay, does this works for you ? here are my toughts:

The user should only be considered by the choice of Inertia Parameters Antropometric Table Item such as Dumas2007.Thigh for example.
thus, at that stage we need to be able to pass the length computed just before ("v = rp -rd") to compute the inertia parameters from the identified length.

What do you think ?

Also, if we uncomment this feature, we need to test to maintain it 👍

model["THIGH"].set_inertia_paramters(
mass=
center_of_mass_inertia=
inertia=
)

def set_inertial_parameters(
self,
mass: float,
center_of_mass: np.ndarray,
inertia_matrix: np.ndarray,
transformation_matrix: TransformationMatrixType,
):
self._natural_inertial_parameters = NaturalInertialParameters.from_cartesian_inertial_parameters(
mass=mass,
center_of_mass=center_of_mass,
inertia_matrix=inertia_matrix,
inertial_transformation_matrix=self.compute_transformation_matrix(transformation_matrix),
)
self._mass = mass
self._natural_center_of_mass = self._natural_inertial_parameters.natural_center_of_mass
self._natural_pseudo_inertia = self.natural_pseudo_inertia
self._mass_matrix = self._natural_inertial_parameters.mass_matrix

@ANaaim
Copy link
Contributor Author

ANaaim commented Nov 7, 2023

Do not forget to use
_update_mass_matrix from bionc/biomechanical model after seting inertia

@ANaaim
Copy link
Contributor Author

ANaaim commented Jan 12, 2024

Should i still work on this ?

@Ipuch
Copy link
Owner

Ipuch commented Jan 12, 2024

I think this PR should be closed as this is not the spirit of the model creation to allow passing numerical values. But mostly to pass the numerical values after the "kinematic model only was updated", like I said in my last comment.

model["THIGH"].set_inertia_paramters(
mass=1,
center_of_mass_inertia=[1,2,3],
inertia=[1,2,3,4,5,6],
)

def set_inertial_parameters(
self,
mass: float,
center_of_mass: np.ndarray,
inertia_matrix: np.ndarray,
transformation_matrix: TransformationMatrixType,
):
self._natural_inertial_parameters = NaturalInertialParameters.from_cartesian_inertial_parameters(
mass=mass,
center_of_mass=center_of_mass,
inertia_matrix=inertia_matrix,
inertial_transformation_matrix=self.compute_transformation_matrix(transformation_matrix),
)
self._mass = mass
self._natural_center_of_mass = self._natural_inertial_parameters.natural_center_of_mass
self._natural_pseudo_inertia = self.natural_pseudo_inertia
self._mass_matrix = self._natural_inertial_parameters.mass_matrix

I don't remember if I implemented it.
If yes, this PR should be closed, if No, you could revert some commit of this PR and start from here or open a new one.

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

Successfully merging this pull request may close these issues.

None yet

2 participants