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

Optional Numpy Compute Backend #1051

Open
wants to merge 39 commits into
base: og-develop
Choose a base branch
from
Open

Optional Numpy Compute Backend #1051

wants to merge 39 commits into from

Conversation

cremebrule
Copy link
Contributor

No description provided.

omnigibson/robots/active_camera_robot.py Show resolved Hide resolved
omnigibson/utils/config_utils.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Base automatically changed from asset-conversion to og-develop December 12, 2024 23:51
Comment on lines +335 to +336
target_pos = cb.copy(control_dict[f"{self.task_name}_pos_relative"])
target_quat = cb.copy(control_dict[f"{self.task_name}_quat_relative"])
Copy link
Member

Choose a reason for hiding this comment

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

why is copying needed?

)

@property
def command_dim(self):
return IK_MODE_COMMAND_DIMS[self.mode]


import torch as th
Copy link
Member

Choose a reason for hiding this comment

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

why is it imported here?

Comment on lines +444 to +447
# Set these as part of the backend values
setattr(_ComputeBackend, "compute_ik_qpos", None)
setattr(_ComputeTorchBackend, "compute_ik_qpos", _compute_ik_qpos_torch)
setattr(_ComputeNumpyBackend, "compute_ik_qpos", _compute_ik_qpos_numpy)
Copy link
Member

Choose a reason for hiding this comment

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

hmm so why don't we define these functions in _ComputeTorch/NumpyBackend?

Comment on lines +314 to +315
@jit(nopython=True)
def numba_ix(arr, rows, cols):
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is significantly faster than np.meshgrid?

Comment on lines +121 to +124
def _generate_default_command_output_limits(self):
command_output_limits = super()._generate_default_command_output_limits()

return cb.mean(command_output_limits[0]), cb.mean(command_output_limits[1])
Copy link
Member

Choose a reason for hiding this comment

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

This only works for the smoothing mode? For binary mode, it's command_output_limits = (-1.0, 1.0) and for independent mode, it's just super()._generate_default_command_output_limits()?

@@ -58,7 +57,7 @@ def __init__(
applied
"""
# Store values
self._default_command = th.zeros(len(dof_idx)) if default_command is None else default_command
self._default_command = cb.zeros(len(dof_idx)) if default_command is None else default_command
Copy link
Member

Choose a reason for hiding this comment

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

wait so self._default_command still has the same dimension as dof_idx instead of 0?

Copy link
Member

Choose a reason for hiding this comment

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

TODO: update to default_goal

Comment on lines +415 to +419
ee_ang_vel_err=cb.as_float32(
cb.T.quat2axisangle(
cb.T.quat_multiply(cb.T.axisangle2quat(-ee_vel[3:]), cb.T.axisangle2quat(base_ang_vel))
)
),
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe compute this first and then feed in? Also should we use orientation_error here or is this different?
Or is it cleaner to just feed in ee_ang_vel and do this computation within the compute_osc_torques function?

omnigibson/robots/holonomic_base_robot.py Show resolved Hide resolved


@jit(nopython=True)
def mat2quat_batch(rmat):
Copy link
Member

Choose a reason for hiding this comment

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

figure out if this is used

_NEXT_AXIS = [1, 2, 0, 1]

# map axes strings to/from tuples of inner axis, parity, repetition, frame
_AXES2TUPLE = {
Copy link
Member

@ChengshuLi ChengshuLi Dec 16, 2024

Choose a reason for hiding this comment

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

make sure one-to-one correspondence with transform_utils.py.

Add a comment at the top of the file to explain that: by default, we use scipy, but we optionally implement numba versions for functions that are often called in "batch" mode?

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.

3 participants