[WIP] #76 Add General Spherical joint #85

ANaaim · 2023-09-05T11:52:39Z

Linked issue related to your pull request using Option in model.add_joint with JointType.SPHERICAL #76
Used the tag [WIP] or [RTR] for on-going changes, or for ready to review
[In progress ] Did you write new tests to cover your changes?
[In progress ] Did you write a proper documentation (docstrings and ReadMe)
[X ] Please black your code black . -l120
make sure the test still pass after solving comments.
build new test to cover the changes.

This change is

Ipuch

Here is my little comments.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ANaaim)

bionc/bionc_casadi/enums.py line 21 at r1 (raw file):

    UNIVERSAL = Joint.Universal
    SPHERICAL = Joint.Spherical
    GENERAL_SPHERICAL = Joint.GeneralSpherical

to be removed

bionc/bionc_casadi/joint.py line 415 at r1 (raw file):

            ), self.child_constraint_jacobian_derivative(Qdot_parent, Qdot_child)

    class GeneralSpherical(JointBase):

Like said in the issue #76 , we should merge the feature of the existing class Spherical.
The default behavior should take parent and child as default points, or it should use the specified points, if any.

bionc/bionc_casadi/joint.py line 438 at r1 (raw file):

            self.nb_constraints = 3
            self.parent_point = parent.marker_from_name(parent_point)

In the future class

self.parent_point = (
    NaturalMarker(  
        name=f"{self.name}_parent_point",  
        position=NaturalVector.distal(),  
        is_technical=False,  
        is_anatomical=True,  
    )  
 if parent_point is None else parent.marker_from_name(parent_point)
)

self.parent_point = (
    NaturalMarker(  
        name=f"{self.name}_child_point",  
        position=NaturalVector.proximal(),  
        is_technical=False,  
        is_anatomical=True,  
    )  
 if parent_point is None else parent.marker_from_name(parent_point)
)

bionc/bionc_casadi/joint.py line 439 at r1 (raw file):

            self.nb_constraints = 3
            self.parent_point = parent.marker_from_name(parent_point)
            self.child_point = child.marker_from_name(child_point)

Explained over this.

bionc/bionc_casadi/joint.py line 470 at r1 (raw file):

        ) -> MX:
            K_k_parent = MX.zeros((self.nb_constraints, 12))
            K_k_parent[:3, 6:9] = self.parent_point.interpolation_matrix

I think the slicing should be [:3, : ]

bionc/bionc_casadi/joint.py line 478 at r1 (raw file):

        ) -> MX:
            K_k_child = MX.zeros((self.nb_constraints, 12))
            K_k_child[:3, 3:6] = -self.child_point.interpolation_matrix

idem

bionc/bionc_numpy/enums.py line 21 at r1 (raw file):

    UNIVERSAL = Joint.Universal
    SPHERICAL = Joint.Spherical
    GENERAL_SPHERICAL = Joint.GeneralSpherical

to be removed

bionc/bionc_numpy/joint.py line 513 at r1 (raw file):

            self.nb_constraints = 3
            self.parent_point = parent.marker_from_name(parent_point)
            self.child_point = child.marker_from_name(child_point)

same comment as for casadi

bionc/bionc_numpy/joint.py line 544 at r1 (raw file):

        ) -> np.ndarray:
            K_k_parent = np.zeros((self.nb_constraints, 12))
            K_k_parent[:3, 6:9] = self.parent_point.interpolation_matrix

same slicing error

bionc/bionc_numpy/joint.py line 552 at r1 (raw file):

        ) -> np.ndarray:
            K_k_child = np.zeros((self.nb_constraints, 12))
            K_k_child[:3, 3:6] = -self.child_point.interpolation_matrix

same slicing error

bionc/bionc_numpy/joint.py line 611 at r1 (raw file):

                The joint as a mx joint
            """
            # TODO: implement this in joint casadi

should be fined to remove if Done, I guess.

Ipuch

Almost done ! I like the fact you spotted some errors in my docstrings 🙈
Please clic on the violet button "Reviewable", to write "Done" under each comment that has been treated.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ANaaim)

bionc/bionc_casadi/joint.py line 415 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Like said in the issue #76 , we should merge the feature of the existing class Spherical.
The default behavior should take parent and child as default points, or it should use the specified points, if any.

Please write "Done" under each comment is treated, so I can remove the comments when the action has been done.

bionc/bionc_casadi/joint.py line 342 at r2 (raw file):

        ):
            super(Joint.Spherical, self).__init__(
                name, parent, child, index, parent_point, child_point, projection_basis, parent_basis, child_basis

"parent_point", "child_point" have to be removed, that's why the tests are not passing.
"Super" calls the parent class JointBase that has common entries for every joint. As all joints doesn't have a parent and child point yet.

It's not relevant to initialize them now. But this would be a great idea in the future !
Then, to initialize parent and child point with proximal and distal point would be automated for every joint, in the parent class JointBase to avoid redundancy.

So, can you remove it for now? And make your PR pass.
Can you open an issue about it ? :)

ANaaim · 2023-09-12T06:30:46Z

bionc/bionc_casadi/joint.py line 415 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Please write "Done" under each comment is treated, so I can remove the comments when the action has been done.

Done

Ipuch

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ANaaim)

Ipuch

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ANaaim)

ANaaim · 2023-09-13T09:20:09Z

bionc/bionc_numpy/joint.py line 513 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same comment as for casadi

Done

ANaaim · 2023-09-13T09:20:17Z

bionc/bionc_numpy/joint.py line 544 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same slicing error

Done.

ANaaim · 2023-09-13T09:20:29Z

bionc/bionc_numpy/joint.py line 611 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

should be fined to remove if Done, I guess.

Done

ANaaim · 2023-09-13T09:23:45Z

bionc/bionc_casadi/joint.py line 342 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

"parent_point", "child_point" have to be removed, that's why the tests are not passing.
"Super" calls the parent class JointBase that has common entries for every joint. As all joints doesn't have a parent and child point yet.

It's not relevant to initialize them now. But this would be a great idea in the future !
Then, to initialize parent and child point with proximal and distal point would be automated for every joint, in the parent class JointBase to avoid redundancy.

So, can you remove it for now? And make your PR pass.

Can you open an issue about it ? :)

Done (Both)

ANaaim

Done

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

bionc/bionc_casadi/joint.py line 438 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

In the future class

self.parent_point = (
    NaturalMarker(  
        name=f"{self.name}_parent_point",  
        position=NaturalVector.distal(),  
        is_technical=False,  
        is_anatomical=True,  
    )  
 if parent_point is None else parent.marker_from_name(parent_point)
)

self.parent_point = (
    NaturalMarker(  
        name=f"{self.name}_child_point",  
        position=NaturalVector.proximal(),  
        is_technical=False,  
        is_anatomical=True,  
    )  
 if parent_point is None else parent.marker_from_name(parent_point)
)

Done.

bionc/bionc_casadi/joint.py line 439 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Explained over this.

Done.

bionc/bionc_casadi/joint.py line 470 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I think the slicing should be [:3, : ]

Done.

bionc/bionc_casadi/joint.py line 478 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

idem

Done.

bionc/bionc_numpy/joint.py line 552 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

same slicing error

Done.

bionc/bionc_casadi/enums.py line 21 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

to be removed

Done.

bionc/bionc_numpy/enums.py line 21 at r1 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

to be removed

Done.

Ipuch

black, and ready to merge I think !!

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ANaaim)

tests/test_joints.py line 100 at r5 (raw file):

        assert joint.nb_joint_dof == 3
        assert joint.nb_constraints == 3

Just put a comment to explain why we need two joints, or name them better:
"joint_distal_proximal" and "joint_custom_location".

tests/test_joints.py line 706 at r5 (raw file):

#
# jacobian_mx = j_jacobian_func(np.arange(24)).toarray()
# def joint_spherical_not_distal_proximal():

plz remove the last line. :)

ANaaim · 2023-09-13T14:53:45Z

tests/test_joints.py line 706 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

plz remove the last line. :)

Done

ANaaim · 2023-09-13T14:56:13Z

tests/test_joints.py line 100 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Just put a comment to explain why we need two joints, or name them better:
"joint_distal_proximal" and "joint_custom_location".

Done

ANaaim · 2023-09-13T14:56:41Z

Done

Ipuch

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ANaaim)

ANaaim added 3 commits September 1, 2023 14:40

Adding the GeneralSpherical joint into joint numpy

43ed374

GeneralSpherical joint seems functional

a8b107b

Blacked

a29554f

Ipuch requested changes Sep 8, 2023

View reviewed changes

ANaaim added 4 commits September 11, 2023 11:51

Fusion of General_Spherical into Spherical

e47fbce

We remove the GeneralSpherical Joint

32fc29b

Blacked

632e16b

Modification to respect BaseJoint variable and Natural marker variable

03c5174

Ipuch requested changes Sep 11, 2023

View reviewed changes

Ipuch reviewed Sep 12, 2023

View reviewed changes

ANaaim added 2 commits September 12, 2023 17:26

Squeeze the constraint + transfer the name of marker to casadi + Blacked

50bc1fb

V1 new_test

6aac307

Ipuch reviewed Sep 12, 2023

View reviewed changes

ANaaim mentioned this pull request Sep 13, 2023

Adaptation of the Joint Base Class to integrate parent_point and child_point (eventually ?) #86

Open

ANaaim commented Sep 13, 2023

View reviewed changes

Ipuch requested changes Sep 13, 2023

View reviewed changes

Blacked

0f60fcf

Correction to better differentiate the two type of spherical joint

9df9fa9

Ipuch approved these changes Sep 13, 2023

View reviewed changes

Ipuch merged commit d7a9a92 into Ipuch:main Sep 13, 2023
1 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] #76 Add General Spherical joint #85

[WIP] #76 Add General Spherical joint #85

ANaaim commented Sep 5, 2023 •

edited by Ipuch

Loading

Ipuch left a comment

Ipuch left a comment

ANaaim commented Sep 12, 2023

Ipuch left a comment

Ipuch left a comment

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim left a comment

Ipuch left a comment

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

Ipuch left a comment

[WIP] #76 Add General Spherical joint #85

[WIP] #76 Add General Spherical joint #85

Conversation

ANaaim commented Sep 5, 2023 • edited by Ipuch Loading

Ipuch left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

ANaaim commented Sep 12, 2023

Ipuch left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim left a comment

Choose a reason for hiding this comment

Ipuch left a comment

Choose a reason for hiding this comment

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

ANaaim commented Sep 13, 2023

Ipuch left a comment

Choose a reason for hiding this comment

ANaaim commented Sep 5, 2023 •

edited by Ipuch

Loading