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

[RTR] new initialization methods Issue #101 #132

Merged
merged 22 commits into from
Mar 15, 2024
Merged

Conversation

aaiaueil
Copy link
Contributor

@aaiaueil aaiaueil commented Jan 11, 2024

  • Linked issue related to your pull request using '#XXX'
  • 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 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @aaiaueil)


bionc/bionc_numpy/initial_guess_mode_type.py line 8 at r1 (raw file):

    USER_PROVIDED = ("UserProvided",)
    USER_PROVIDED_FIRST_FRAME_ONLY = ("UserProvidedFirstFrameOnly",)
    FROM_FIRST_FRAME_MARKERS = "FromFirstFrameMarkers"

Put this in enums.py


bionc/bionc_numpy/inverse_kinematics.py line 72 at r1 (raw file):

        self,
        model: BiomechanicalModel,
        initial_guess_mode: Enum,

What you wrote is not wrong but this would be more precise.

initial_guess_mode: InitialGuessModeType,

bionc/bionc_numpy/inverse_kinematics.py line 74 at r1 (raw file):

        initial_guess_mode: Enum,
        Q_init: np.ndarray | NaturalCoordinates = None,
        initialize_markers: np.ndarray = None,

I believe this new entry is a mistake, this is a mistake to remove !!

Code quote:

        initialize_markers: np.ndarray = None,

bionc/bionc_numpy/inverse_kinematics.py line 132 at r1 (raw file):

                raise NotImplementedError("Not available yet, please provide Q_init")
            else:
                self.Q_init = self.model.Q_from_markers(self.experimental_markers[:, :, :])

You deleted this line that is absolutely major !!
It computes the Q from markers, and it can be used without providing 'initialize_markers'
in the case of "FROM_MARKERS", this function needs to be used.


bionc/bionc_numpy/inverse_kinematics.py line 253 at r1 (raw file):

            self.initialize_markers = initialize_markers

        self.initial_guess_mode = initial_guess_mode

Please displace this section of code in the method solve.
and encapsulate it in a method name ̀get_Q_init_from_initial_guess_mode(...) `

Code quote:

        if initial_guess_mode == InitialGuessModeType.USER_PROVIDED:
            if Q_init is None:
                raise ValueError("Please provide Q_init if you want to use USER_PROVIDED mode")
            if Q_init.shape[1] != self.nb_frames:
                raise ValueError("Please make sure Q_init contains all the frames")
            self.Q_init = Q_init

        elif initial_guess_mode == InitialGuessModeType.USER_PROVIDED_FIRST_FRAME_ONLY:
            if Q_init is None:
                raise ValueError("Please provide Q_init if you want to use USER_PROVIDED_FIRST_FRAME_ONLY mode")
            if len(Q_init.shape) > 1:
                raise ValueError("Please provide only the first frame for Q_init")
            if self._frame_per_frame == False:
                raise ValueError("Please set frame_per_frame to True")
            self.Q_init = Q_init

        elif initial_guess_mode == InitialGuessModeType.FROM_CURRENT_MARKERS:
            if initialize_markers is None:
                raise ValueError("Please provide initialize_markers in order to initialize the optimization")
            if initialize_markers.shape[2] != self.nb_frames:
                raise ValueError("Please make sure initalize_markers contains all the frames")
            if experimental_heatmaps is not None:
                raise ValueError(
                    "Q_init cannot be computed from markers using heatmap data, please either provide marker data or change initialization mode"
                )
            self.initialize_markers = initialize_markers

        elif initial_guess_mode == InitialGuessModeType.FROM_FIRST_FRAME_MARKERS:
            if initialize_markers is None:
                raise ValueError("Please provide initialize_markers in order to initialize the optimization")
            if len(initialize_markers.shape) > 2:
                raise ValueError("Please provide only the first frame for markers")
            if experimental_heatmaps is not None:
                raise ValueError(
                    "Q_init cannot be computed from markers using heatmap data, please either provide marker data or change initialization mode"
                )
            if self._frame_per_frame == False:
                raise ValueError("Please set frame_per_frame to True")
            self.initialize_markers = initialize_markers

        self.initial_guess_mode = initial_guess_mode

bionc/bionc_numpy/inverse_kinematics.py line 317 at r1 (raw file):

    def solve(
        self, method: str = "ipopt", options: dict = None
    ) -> np.ndarray:  # initial_guess_mode: InitialGuessModeType.USER_PROVIDED,

Yes this should be somewhere here. keep initial_guess_mode as an entry of the method


bionc/bionc_numpy/inverse_kinematics.py line 378 at r1 (raw file):

                        Q_init = self.Q_init
                    else:
                        Q_init = self.model.Q_from_markers(self.initialize_markers[:, :, f : f + 1])

This should be generic for both for the two if cases for example:

InitialGuessModeType.USER_PROVIDED
InitialGuessModeType.FROM_CURRENT_MARKERS

should work both for solve_frame_per_frame = True and False

Code quote:

                if self.initial_guess_mode == InitialGuessModeType.USER_PROVIDED:
                    Q_init = self.Q_init[:, f : f + 1]
                if self.initial_guess_mode == InitialGuessModeType.FROM_CURRENT_MARKERS:
                    Q_init = self.model.Q_from_markers(self.initialize_markers[:, :, f : f + 1])
                if self.initial_guess_mode == InitialGuessModeType.USER_PROVIDED_FIRST_FRAME_ONLY:
                    if f == 0:
                        Q_init = self.Q_init
                    else:
                        Q_init = Qopt[:, f - 1 : f]
                if self.initial_guess_mode == InitialGuessModeType.FROM_FIRST_FRAME_MARKERS:
                    if f == 0:
                        Q_init = self.Q_init
                    else:
                        Q_init = self.model.Q_from_markers(self.initialize_markers[:, :, f : f + 1])

Copy link
Contributor Author

@aaiaueil aaiaueil 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: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @Ipuch)


bionc/bionc_numpy/initial_guess_mode_type.py line 8 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Put this in enums.py

I get a circular import problem with import Joint when I put both classes in the same file. For now I keep them separated


bionc/bionc_numpy/inverse_kinematics.py line 72 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

What you wrote is not wrong but this would be more precise.

initial_guess_mode: InitialGuessModeType,

Done.


bionc/bionc_numpy/inverse_kinematics.py line 74 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I believe this new entry is a mistake, this is a mistake to remove !!

Done.


bionc/bionc_numpy/inverse_kinematics.py line 132 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

You deleted this line that is absolutely major !!
It computes the Q from markers, and it can be used without providing 'initialize_markers'
in the case of "FROM_MARKERS", this function needs to be used.

I kept the if loop to check that Q_init is not None for heatmaps, but self.model.Q_from_markers(...) is now used in get_Q_init_from_initial_guess_mode.


bionc/bionc_numpy/inverse_kinematics.py line 253 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Please displace this section of code in the method solve.
and encapsulate it in a method name ̀get_Q_init_from_initial_guess_mode(...) `

Done.


bionc/bionc_numpy/inverse_kinematics.py line 317 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Yes this should be somewhere here. keep initial_guess_mode as an entry of the method

Done.


bionc/bionc_numpy/inverse_kinematics.py line 378 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

This should be generic for both for the two if cases for example:

InitialGuessModeType.USER_PROVIDED
InitialGuessModeType.FROM_CURRENT_MARKERS

should work both for solve_frame_per_frame = True and False

Is this urgent? I'm still not sure I understand how solving for all frames works and am running a bit out of time

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 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aaiaueil)


bionc/bionc_numpy/inverse_kinematics.py line 2 at r2 (raw file):

from typing import Callable
from enum import Enum

Makes no sens.


bionc/bionc_numpy/inverse_kinematics.py line 72 at r2 (raw file):

        self,
        model: BiomechanicalModel,
        Q_init: np.ndarray | NaturalCoordinates = None,

Q_init as an entry of solve ?


bionc/bionc_numpy/inverse_kinematics.py line 324 at r2 (raw file):

    def solve(
        self, initial_guess_mode: InitialGuessModeType, method: str = "ipopt", options: dict = None
    ) -> np.ndarray:

Q_init as entry.

ik = InverseKinematics(experimental_markers)
ik.solve(Q_init)
ik.solve(Q_init2)

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

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

Project coverage is 83.84%. Comparing base (e423b9c) to head (5696fb8).
Report is 24 commits behind head on main.

Files Patch % Lines
bionc/bionc_numpy/inverse_kinematics.py 89.61% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   83.51%   83.84%   +0.33%     
==========================================
  Files          91       93       +2     
  Lines        5842     5968     +126     
==========================================
+ Hits         4879     5004     +125     
- Misses        963      964       +1     
Flag Coverage Δ
unittests 83.84% <91.75%> (+0.33%) ⬆️

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.

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 6 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aaiaueil)

@aaiaueil aaiaueil changed the title [RTR] new initialization methods Issue #101 [WIP] new initialization methods Issue #101 Jan 19, 2024
think I caught a bug in inverse_kinematics for FROM_FIRST_FRAME_MARKERS
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 all commit messages.
Reviewable status: 7 of 10 files reviewed, all discussions resolved (waiting on @Ipuch)


bionc/bionc_numpy/inverse_kinematics.py line 264 at r4 (raw file):

            )

            return self.model.Q_from_markers(experimental_markers[:, :, frame_slice])

Why removing this, this makes no sens to have a Q_init of size [nb_q x nb_frames] when we have use the markers of the first frame to estimate the Q of the first frame too.

It means the output size of this function, which is meant to get Q_init from the initial guess mode should respect the terminology of the provided enum.

If I provide InitialGuessModeType.FROM_FIRST_FRAME_MARKERS, I expect the size of Q_init to be [nb_q x 1] because the next estimates of frame too will be evaluated from last frame.

Code quote:

            frame_slice = (
                slice(0, 1) if initial_guess_mode == InitialGuessModeType.FROM_FIRST_FRAME_MARKERS else slice(None)
            )

            return self.model.Q_from_markers(experimental_markers[:, :, frame_slice])

bionc/bionc_numpy/inverse_kinematics.py line 284 at r4 (raw file):

                raise ValueError("Please provide Q_init if you want to use USER_PROVIDED_FIRST_FRAME_ONLY mode.")
            if len(Q_init.squeeze().shape) > 1:
                raise ValueError("Please provide only the first frame for Q_init.")

I disagree too because Q_init should be [nb_q x 1] or [nb_q] in that case ! That's may explain an error that wasn't raised when designing your tests.

Code quote:

            if len(Q_init.squeeze().shape) > 1:
                raise ValueError("Please provide only the first frame for Q_init.")

examples/model_creation/marker_model.py line 1 at r5 (raw file):

import os

Why adding another model in model creation? Why not using one already existing ? If you give me a rationale for this, I can pass on this.
But if this was not absolutely necessary, please use another model in the new tests, and change the expected_values.


examples/model_creation/marker_model.py line 32 at r5 (raw file):

def harrington2007(RASIS: np.ndarray, LASIS: np.ndarray, RPSIS: np.ndarray, LPSIS: np.ndarray) -> tuple:

If the model is necessary, this function is already implemented elsewhere in the lower_limb model I believe, please use/import it instead of duplicating code.


tests/test_initial_guess_mode.py line 6 at r5 (raw file):

from bionc import InverseKinematics, NaturalCoordinates
from bionc.bionc_numpy.enums import *

Please import only what is useful, * is considered a bad practice when developing software.


tests/test_initial_guess_mode.py line 10 at r5 (raw file):

Q_initialize = NaturalCoordinates(

Capitalize all the letters of Q_initialize. Constants and global variables should declare all capitalized according to PEP8 style.


tests/test_initial_guess_mode.py line 285 at r5 (raw file):

    )
    ik.solve(
        Q_init=Q_initialize, initial_guess_mode=InitialGuessModeType.USER_PROVIDED_FIRST_FRAME_ONLY, method="ipopt"

Should it be Q_init=Q_initialize[:,0:1]? Because we only need one frame here (?)

Thus, I believe, this case should throw/raise an error.

@Ipuch
Copy link
Owner

Ipuch commented Mar 1, 2024

@aaiaueil up

@aaiaueil
Copy link
Contributor Author

Reviewed all commit messages.
Reviewable status: 7 of 10 files reviewed, all discussions resolved (waiting on @Ipuch)

bionc/bionc_numpy/inverse_kinematics.py line 264 at r4 (raw file):

            )

            return self.model.Q_from_markers(experimental_markers[:, :, frame_slice])

Why removing this, this makes no sens to have a Q_init of size [nb_q x nb_frames] when we have use the markers of the first frame to estimate the Q of the first frame too.

It means the output size of this function, which is meant to get Q_init from the initial guess mode should respect the terminology of the provided enum.

If I provide InitialGuessModeType.FROM_FIRST_FRAME_MARKERS, I expect the size of Q_init to be [nb_q x 1] because the next estimates of frame too will be evaluated from last frame.

Code quote:

            frame_slice = (
                slice(0, 1) if initial_guess_mode == InitialGuessModeType.FROM_FIRST_FRAME_MARKERS else slice(None)
            )

            return self.model.Q_from_markers(experimental_markers[:, :, frame_slice])

It seems that in the current state of the code it's actually needed to have Q_init of size [nb_q x nb_frames] : first in update_initial_guess there is (line 396)

Code quote:

Q_init[:, frame + 1 : frame + 2] = Qopt[:, frame : frame + 1]

where Q_init should be of size [nb_q x nb_frames], and there is also in solve_frame_per_frame (line 372):

Code quote:

r, success = _solve_nlp(method, nlp, Q_init[:, f], lbg, ubg, options)

That's actually in line 372 that I had an error when using slice(0,1)

I see two options there : either we keep Q_init of size [nb_q x nb_frames], and the values of Q_init are modified frame by frame in update_initial_guess, or we initialize Q_init of size [nb_q x 1] and then append values frame by frame. I think option #1 is easier but your choice!

Copy link
Contributor Author

@aaiaueil aaiaueil 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: 7 of 10 files reviewed, all discussions resolved (waiting on @Ipuch)


bionc/bionc_numpy/inverse_kinematics.py line 2 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Makes no sens.

Why? do you mean numpy? It is used several times


bionc/bionc_numpy/inverse_kinematics.py line 72 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Q_init as an entry of solve ?

yes, it shouldn't?


bionc/bionc_numpy/inverse_kinematics.py line 324 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Q_init as entry.

ik = InverseKinematics(experimental_markers)
ik.solve(Q_init)
ik.solve(Q_init2)

I'm not sure I understand, that's how it is coded at the moment.

@aaiaueil aaiaueil closed this Mar 13, 2024
@aaiaueil aaiaueil reopened this Mar 13, 2024
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 2 of 3 files at r5, all commit messages.
Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on @aaiaueil)


bionc/bionc_numpy/inverse_kinematics.py line 72 at r2 (raw file):

Previously, aaiaueil wrote…

yes, it shouldn't?

Outdated


bionc/bionc_numpy/inverse_kinematics.py line 324 at r2 (raw file):

Previously, aaiaueil wrote…

I'm not sure I understand, that's how it is coded at the moment.

outdated - Already resolved

@aaiaueil aaiaueil changed the title [WIP] new initialization methods Issue #101 [RTR] new initialization methods Issue #101 Mar 13, 2024
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 all commit messages.
Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @aaiaueil)


tests/test_initial_guess_mode.py line 18 at r6 (raw file):

    model = module.model_creation_from_measured_data()
    markers = Markers.from_c3d(c3d_filename, usecols=model.marker_names_technical).to_numpy()
    Q_INITIALIZE = model.Q_from_markers(markers[:, :, :])

no more needed to capitalize as this is not a global variable. Q_initialize

Code quote:

Q_INITIALIZE

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: 8 of 10 files reviewed, 7 unresolved discussions (waiting on @aaiaueil)


tests/test_initial_guess_mode.py line 301 at r6 (raw file):

test_user_provided()

this line should not be here

@aaiaueil
Copy link
Contributor Author

Redundancy detected :

In get_Q_init_from_initial_guess_mode lines 245 - 250

if experimental_markers is None:
    raise ValueError("Please provide experimental_markers in order to initialize the optimization")
if self.experimental_heatmaps is not None:
    raise ValueError(
        "Q_init cannot be computed from markers using heatmap data, please either provide marker data or change initialization mode"
    )

Seems to be redundant with line 102 in init

if experimental_markers is None and experimental_heatmaps is None:
    raise ValueError("Please feed experimental data, either marker or heatmap data")

If there is no data, either markers or heatmap, then ValueError("Please feed experimental data, either marker or heatmap data") is raised first, and it's not possible to have both markers and heatmap data provided.

@aaiaueil
Copy link
Contributor Author

Impossible error :

  if (
      initial_guess_mode == InitialGuessModeType.FROM_CURRENT_MARKERS
      and experimental_markers.shape[2] != self.nb_frames
  ):
      raise ValueError("Please make sure initalize_markers contains all the frames")

Impossible to raise this error as self.nb_frames is defined as self.nb_frames = self.experimental_markers.shape[2]

To me this error is not needed

Copy link
Contributor Author

@aaiaueil aaiaueil 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: 8 of 10 files reviewed, 7 unresolved discussions (waiting on @Ipuch)


bionc/bionc_numpy/inverse_kinematics.py line 264 at r4 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Why removing this, this makes no sens to have a Q_init of size [nb_q x nb_frames] when we have use the markers of the first frame to estimate the Q of the first frame too.

It means the output size of this function, which is meant to get Q_init from the initial guess mode should respect the terminology of the provided enum.

If I provide InitialGuessModeType.FROM_FIRST_FRAME_MARKERS, I expect the size of Q_init to be [nb_q x 1] because the next estimates of frame too will be evaluated from last frame.

this was fixed in my latest commit


bionc/bionc_numpy/inverse_kinematics.py line 284 at r4 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I disagree too because Q_init should be [nb_q x 1] or [nb_q] in that case ! That's may explain an error that wasn't raised when designing your tests.

Fixed in latest commit as well


examples/model_creation/marker_model.py line 1 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Why adding another model in model creation? Why not using one already existing ? If you give me a rationale for this, I can pass on this.
But if this was not absolutely necessary, please use another model in the new tests, and change the expected_values.

Model was deleted


examples/model_creation/marker_model.py line 32 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

If the model is necessary, this function is already implemented elsewhere in the lower_limb model I believe, please use/import it instead of duplicating code.

Model was deleted so comment not up to date


tests/test_initial_guess_mode.py line 10 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Capitalize all the letters of Q_initialize. Constants and global variables should declare all capitalized according to PEP8 style.

Done.


tests/test_initial_guess_mode.py line 285 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Should it be Q_init=Q_initialize[:,0:1]? Because we only need one frame here (?)

Thus, I believe, this case should throw/raise an error.

Woops, my bad. I added a raise ValueError in inverse_kinematics.py and some tests in test_initial_guess_mode


tests/test_initial_guess_mode.py line 18 at r6 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

no more needed to capitalize as this is not a global variable. Q_initialize

Done.


tests/test_initial_guess_mode.py line 301 at r6 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

this line should not be here

Done.

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 2 of 2 files at r7, 1 of 2 files at r9.
Reviewable status: 9 of 11 files reviewed, all discussions resolved (waiting on @aaiaueil)

Copy link

codeclimate bot commented Mar 15, 2024

Code Climate has analyzed commit 5696fb8 and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Duplication 2

View more on Code Climate.

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.

:lgtm:

Reviewed 4 of 4 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aaiaueil)

@Ipuch Ipuch merged commit 7badb8e into Ipuch:main Mar 15, 2024
8 checks passed
@Ipuch Ipuch mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants