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

Add benchmark metrics to simulator #823

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

Add benchmark metrics to simulator #823

wants to merge 38 commits into from

Conversation

yyf20001230
Copy link

initial commit to benchmark metrics

@yyf20001230 yyf20001230 requested a review from cgokmen July 30, 2024 21:44
omnigibson/tasks/task_base.py Outdated Show resolved Hide resolved
float: partial success if supported, -1.0 otherwise
"""
assert self._done is not None, "At least one step() must occur before partial_success can be calculated!"
return len(satisfied) / (len(satisfied) + len(unsatisfied))
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to implement the satisfied etc. things - ask Eric how to do this

omnigibson/envs/env_base.py Outdated Show resolved Hide resolved
omnigibson/envs/env_base.py Outdated Show resolved Hide resolved
omnigibson/envs/env_base.py Outdated Show resolved Hide resolved
super().__init__()
self._reward = 0
self.initialized = False
self.state_cache = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Start this as None

Comment on lines 48 to 49
if self.measure_work:
self._reward = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these two lines

if not self.measure_work:
self.state_cache = new_state_cache

self._reward += work_metric
Copy link
Contributor

Choose a reason for hiding this comment

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

Here do self._reward = work_metric if self.measure_work else (self._reward + work_metric)

omnigibson/reward_functions/energy_metric.py Outdated Show resolved Hide resolved
omnigibson/tasks/task_base.py Outdated Show resolved Hide resolved
@yyf20001230 yyf20001230 changed the title Add benchmark metrics Add benchmark metrics to simulator Jul 30, 2024
omnigibson/envs/env_base.py Outdated Show resolved Hide resolved
Comment on lines 536 to 537
# record the start time of the simulation step in the beginning of the step
self._cur_sim_start_ts = time.perf_counter()
Copy link
Member

Choose a reason for hiding this comment

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

should we move these logic into the metrics themselves? e.g. the env class just call a bunch of pre_step for each of the metrics, something like this:

for metric in self.task.metrics:
    metric.pre_step()

omnigibson/metrics/energy_metric.py Outdated Show resolved Hide resolved
Comment on lines 49 to 50
# TODO: this computation is very slow, consider using a more efficient method
# TODO: this method needs to be updated to account for object addition and removal
Copy link
Member

Choose a reason for hiding this comment

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

can we actually finish these two TODOs?

# TODO: this computation is very slow, consider using a more efficient method
# TODO: this method needs to be updated to account for object addition and removal
posrot2 = self.state_cache[linkname]
work_metric += np.linalg.norm(posrot[0] - posrot2[0]) * self.link_masses[linkname]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should consider rotation changes as well, e.g. a weighted sum of translation changes and rotation changes, where the weights are hyperparams that can be configured.

Copy link
Author

Choose a reason for hiding this comment

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

The work done by rotation should be W = torque * rotation. Without knowing torque, I added a rough calculation of the work/energy metric. I am happy to discuss this calculation more specifically with you tomorrow.

position, orientation = T.pose_transform(posrot[0], posrot[1], init_posrot[0], init_posrot[1])
work_metric += np.linalg.norm(orientation) * self.link_masses[linkname] * self.metric_config["rotation"]

omnigibson/tasks/task_base.py Outdated Show resolved Hide resolved
@@ -576,7 +576,7 @@ def get_control_dict(self):
# TODO: Move gravity force computation dummy to this class instead of BaseRobot
fcns["gravity_force"] = lambda: (
ControllableObjectViewAPI.get_generalized_gravity_forces(self.articulation_root_path)
if self.fixed_base
if self.fixed_base or self._dummy is None
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 this line needed?

Copy link
Author

Choose a reason for hiding this comment

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

I believe that self._dummy is None should be there to account for the fact that dummy is not loaded at this stage of the simulator initialization. However, dummy will be loaded later, after the gravity force computation.
I also resolved this TODO, moving the dummy initialization from the BaseRobot to ControllableObject.

omnigibson/tasks/behavior_task.py Outdated Show resolved Hide resolved
tests/test_metric_functions.py Outdated Show resolved Hide resolved
assert metrics["work"] == 0, "Work metric was not reset"


def test_behavior_task_work_metric():
Copy link
Member

Choose a reason for hiding this comment

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

can we add tests for all the metrics, e.g. wall time, energy, steps, etc.?

Copy link
Author

Choose a reason for hiding this comment

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

env.step()
number of time step * action_time_step

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.

4 participants