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

Fix implementation of Transform.inv() #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

allanzhao
Copy link

I've been porting some of my research code to Brax v2, and the performance is very good so far! Our code uses rigid body transforms extensively, and we noticed that Transform.inv() seems to differ from the textbook formula (as mentioned in #297).

This pull request:

  1. changes the implementation of brax.v2.base.Transform.inv() to match tiny-differentiable-simulator and MuJoCo
  2. adds a test case at the end of brax/v2/math_test.py (please feel free to move to an appropriate place)

  - in addition to negating position, transform it into the other
  coordinate frame
@btaba
Copy link
Collaborator

btaba commented Mar 10, 2023

Hi @allanzhao ! Thanks for the PR and documenting the bug!
We removed Transform.inv() in favor of inv_do

brax/brax/v2/base.py

Lines 127 to 129 in 9d5a88e

def inv_do(self, o):
"""Apply the inverse of the transform."""
return _transform_inv_do(o, self)

the reason being that Transform.inv(), as implemented here, is wrt other Transforms, not necessarily wrt a Motion or a Force which is a bit confusing. We were only using Transform.inv_do(Motion), but if you want to change this into a Transform.inv_do(Transform) operator then that would be great!

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

2 participants