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

RNEA method #125

Merged
merged 72 commits into from
Jun 6, 2023
Merged

RNEA method #125

merged 72 commits into from
Jun 6, 2023

Conversation

Tianhuanyu
Copy link
Contributor

@Tianhuanyu Tianhuanyu commented Apr 11, 2023

Description

The RNEA method is added to model module of optas for computing inverseDynamics. Given q, qd, qdd, the function could return a external force vector.

The RNEA is tested with test_rnea.py

The current version of RNEA only supports the robots with:

  1. resolute joints,
  2. the first joint of robot fixed,
  3. no fixed joints except the first joint and the last joint.

Todo

  • Completed implementation of new feature.
  • Add unit-test for the new feature.
  • Add documentation for the new feature.
  • Ensure all unit tests pass.
  • Address all issues raised by unit-tests.
  • Consider adding an example that demonstrates the use of the new feature (not always necessary).
  • Added code reviewer.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tianhuanyu! This is great! I have some feedback (please see comments). Also, you should run black on the files you modify - this will ensure the code formatting is correct.

optas/models.py Outdated Show resolved Hide resolved
optas/models.py Outdated Show resolved Hide resolved
optas/models.py Outdated Show resolved Hide resolved
optas/models.py Outdated Show resolved Hide resolved
optas/models.py Outdated Show resolved Hide resolved
example/TrackingBall.py Outdated Show resolved Hide resolved
@cmower
Copy link
Owner

cmower commented Apr 11, 2023

Please also address the todo list in the PR description above.

@cmower
Copy link
Owner

cmower commented Apr 11, 2023

Could you also make sure to add a unit test in tests/test_models.py. It would be great if you could generate random q, dq, ddq and check the output of your function against the output of PyBullet RNEA method to ensure it is at-least numerically equivalent.

@Tianhuanyu
Copy link
Contributor Author

Thanks @Tianhuanyu! This is great! I have some feedback (please see comments). Also, you should run black on the files you modify - this will ensure the code formatting is correct.

Thanks, @cmower . I try black for reformatting my code. It's fantastic. All the following parts are done within this update.

@Tianhuanyu
Copy link
Contributor Author

Could you also make sure to add a unit test in tests/test_models.py. It would be great if you could generate random q, dq, ddq and check the output of your function against the output of PyBullet RNEA method to ensure it is at-least numerically equivalent.

Sure, but I find there is always a small error between PyBullet output and our RNEA. (around 0.5% or less). Maybe Pybullet uses a different method for computing inverse dynamics or it may consider friction. Currently, the error doesn't have much effect on my application. It maybe a long-term work to find out why the numbers are slightly different.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Tianhuanyu, thanks for the updates. Just a couple more changes.

Also, could you please add a unit test (that compares RobotModel.rnea to PyBullet) into tests/test_models.py. The small error you see is fine. As you mention, PyBullet probably takes into account friction or maybe other things. The test is good to have though because it verifies i) you're implementation is working (i.e. the code runs), and ii) the values it outputs are "pretty much" what we expect. Take a look here for more details about the implementation of RNEA in PyBullet.

optas/models.py Outdated Show resolved Hide resolved
optas/models.py Show resolved Hide resolved
@Tianhuanyu
Copy link
Contributor Author

Hi @Tianhuanyu, thanks for the updates. Just a couple more changes.

Also, could you please add a unit test (that compares RobotModel.rnea to PyBullet) into tests/test_models.py. The small error you see is fine. As you mention, PyBullet probably takes into account friction or maybe other things. The test is good to have though because it verifies i) you're implementation is working (i.e. the code runs), and ii) the values it outputs are "pretty much" what we expect. Take a look here for more details about the implementation of RNEA in PyBullet.

Thanks. @cmower I've submitted the modification for the two minor suggestion. I will submit the testing part as soon as possible.

@cmower
Copy link
Owner

cmower commented Apr 12, 2023

Great, thanks @Tianhuanyu.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tianhuanyu, some last requests. After you've made these changes we should be able to merge this PR :)

# Setup model (with no parameterized joints)
model = optas.RobotModel(urdf_filename=urdf_path)

pb.connect(*[pb.DIRECT])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace line with:

pb.connect(pb.DIRECT)

Comment on lines 43 to 49
def test_calculate_inverse_dynamics_symbol(self):
for _ in range(NUM_RANDOM):
q = optas.SX.sym("q", self.model.ndof)
qd = optas.SX.sym("qd", self.model.ndof)
qdd = optas.SX.sym("qdd", self.model.ndof)
tau1 = self.model.rnea(q, qd, qdd)
assert isinstance(tau1, optas.SX)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to run this in a loop, checking symbolic output only needs to be done once.

)
# print(tau1)
# print(tau2)
assert isclose(tau1.T, tau2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value for tau1 is being returned as an optas.DM variable, and is a vector. I suggest using

assert isclose(tau1.toarray().flatten(), tau2)

toarray converts the optas.DM variable to a numpy array

Comment on lines 68 to 75
def main(q=None):
test = TestRnea()
test.test_calculate_inverse_dynamics_symbol()
test.test_calculate_inverse_dynamics()


if __name__ == "__main__":
main()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove, pytest doesn't require a main function.

NUM_RANDOM = 100


class TestRnea:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this class to tests/test_models.py. No need for the additional script.

optas/models.py Outdated Show resolved Hide resolved
@cmower cmower merged commit b2cbbbe into cmower:master Jun 6, 2023
10 checks passed
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.

None yet

3 participants