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

Exposed function get_joint_motor_torques with unit tests #1585

Merged
merged 22 commits into from
Jan 25, 2022

Conversation

SimarKareer
Copy link
Contributor

@SimarKareer SimarKareer commented Dec 3, 2021

Motivation and Context

This change allows a user in python to get the torques applied by each joint motor. Currently only implemented for non-spherical joints. I exposed this so I could implement a reward function which relies on joint torques.

How Has This Been Tested

I added test cases in tests/test_physics.py. Specifically I moved motors and ensured that the torque values were sufficiently high. Then after the motors reached their target I ensured that torques were sufficiently low.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [x ] I have read the CONTRIBUTING document.
  • [x ] I have completed my CLA (see CONTRIBUTING)
  • [x ] I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 3, 2021
@SimarKareer SimarKareer closed this Dec 3, 2021
@SimarKareer SimarKareer reopened this Dec 3, 2021
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this! A couple of doc comments and a test suggestion:

src/esp/physics/ArticulatedObject.h Outdated Show resolved Hide resolved
*
* @return Array of torques on each joint
*/
std::vector<float> getJointMotorTorques(double fixedTimeStep);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add the param documentation here so the web doc generation will pick it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aclegg3 We would need to add these two new calls to the JS binding then as well.

tests/test_physics.py Outdated Show resolved Hide resolved
src/esp/bindings/PhysicsObjectBindings.cpp Show resolved Hide resolved
src/esp/sim/Simulator.cpp Show resolved Hide resolved
/**
* @brief Get the torques on each joint
*
* @return Array of torques on each joint
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically returns a vector of torques. You can return an array and it will be converted by PyBind11 as a numpy array

@SimarKareer
Copy link
Contributor Author

I think I've addressed all the issues! Lmk what you think. (For some reason this pylint is failing on GitHub but is fine locally)

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Looking good overall. A couple minor corrections:

src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated Show resolved Hide resolved
src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

clang-tidy resolutions

src/esp/physics/ArticulatedObject.h Outdated Show resolved Hide resolved
src/esp/physics/bullet/BulletArticulatedObject.h Outdated Show resolved Hide resolved
SimarKareer and others added 3 commits January 18, 2022 13:31
Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>
Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

New clang-tidy clean-up

src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated Show resolved Hide resolved
src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

You actually want to throw an exception here, not a string right?

SimarKareer and others added 3 commits January 20, 2022 10:01
Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>
Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>
@SimarKareer SimarKareer marked this pull request as ready for review January 25, 2022 16:24
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your contribution, @SimarKareer!
When the tests pass, good to squash-merge. 🎉

@aclegg3 aclegg3 merged commit b276179 into facebookresearch:main Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants