Skip to content

feat: Add impratio to testspeed results#947

Merged
thowell merged 8 commits intogoogle-deepmind:mainfrom
zer-art:feat/testspeed-output-details
Jan 16, 2026
Merged

feat: Add impratio to testspeed results#947
thowell merged 8 commits intogoogle-deepmind:mainfrom
zer-art:feat/testspeed-output-details

Conversation

@zer-art
Copy link
Copy Markdown
Contributor

@zer-art zer-art commented Dec 27, 2025

Fixes #944

Summary

This PR updates mujoco_warp/testspeed.py to include additional simulation details in the final output report.

Changes:

  • Added logging for impratio (checked via mjm.opt.impratio).
  • Added logic to determine and report required collision routines:
    • Primitive: Checked against standard primitive collision pairs.
    • CCD: Checked against convex collision pairs.
    • SDF: Checked via m.has_sdf_geom.

Verification

I tested the changes locally using the aloha_pot scene.

Command:

python mujoco_warp/testspeed.py mujoco_warp/test_data/aloha_pot/scene.xml --nworld 10 --nstep 10

Output:

Data
  nworld: 10 naconmax: 480 njmax: 64

  impratio: 10.0
  collision routines required: primitive, ccd

Checklist

  • I have signed the CLA.
  • My code follows the style guidelines of this project.

Tanisha-Sharma005 added a commit to Tanisha-Sharma005/mujoco_warp that referenced this pull request Dec 30, 2025
Closes google-deepmind#947

- Add collision pairs, contacts, friction dim to testspeed summary
- Matches issue requirements: friction + collision details in final output
- Formatted consistently with existing timing stats
@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 6, 2026

@zer-art thank you for contributing to mujoco warp!

lets focus on just adding impratio information in this pr and we should print the value from the mujoco warp model option m.opt.impratio instead of the mujoco mjmodel option.

additionally lets add a flag --extra_info that is false by default and needs to be true in order to print impratio.

please let us know if you have any questions. thanks!

@zer-art
Copy link
Copy Markdown
Contributor Author

zer-art commented Jan 7, 2026

Summary of Changes

1. Feature: impratio Reporting testspeed.py

Added the --extra_info flag (defaults to False).

Removed the collision routine calculation section to keep the PR atomic and focused.

The script now prints m.opt.impratio only when the --extra_info flag is enabled.

2. Core Update _src/types.py & _src/io.py

Context: I found that the Warp model options (m.opt) previously only stored impratio_invsqrt, making m.opt.impratio inaccessible by default.

Change: I added impratio to the Option struct and populated it in put_model to ensure the benchmark script can access the correct value.

3. Fix: CPU/Mac Compatibility _src/benchmark.py

Context: While verifying these changes locally on macOS (CPU), I encountered a crash because wp.ScopedCapture and wp.ScopedStream were being used unconditionally.

Change: I added a check for wp.get_device().is_cuda to ensure these CUDA-specific context managers are only active when a CUDA device is present.

Verified locally with python -m mujoco_warp.testspeed ... --extra_info. Ready for review!

@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 7, 2026

@zer-art thanks for the update!

  • lets remove changes to all files except testspeed.py
  • lets compute impratio (1.0 / (impratio_invsqrt * impratio_invsqrt)) in the f-string

@zer-art
Copy link
Copy Markdown
Contributor Author

zer-art commented Jan 8, 2026

@thowell Done! I've reverted all changes to _src/types.py, _src/io.py, and _src/benchmark.py.

I also updated testspeed.py to calculate the ratio in-place using the formula: 1.0 / (impratio_invsqrt * impratio_invsqrt).

Heads up on a Mac/CPU Bug: While verifying the revert, I confirmed that the current main branch crashes on macOS because benchmark.py calls CUDA functions unconditionally.

The Error:

File "warp/_src/context.py", line 7304, in capture_begin
    if runtime.driver_version >= (12, 3):
TypeError: '>=' not supported between instances of 'NoneType' and 'tuple'

(This happens because runtime.driver_version is None on CPU devices).

I plan to open a separate Issue to track this bug so we can keep this PR focused strictly on the feature. Let me know if you'd prefer I handle it differently.

Comment thread mujoco_warp/testspeed.py Outdated
Comment thread mujoco_warp/testspeed.py Outdated
@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 9, 2026

@zer-art thank you for the update!

if you are interested in contributing an update to testspeed related to not working with cpu:

  • create a separate pr
  • add a check to testspeed.py for cpu, something like
if "cpu" in (_DEVICE.value, wp.get_device()):
  raise ValueError("testspeed available for gpu only")

thanks!

@zer-art
Copy link
Copy Markdown
Contributor Author

zer-art commented Jan 9, 2026

@thowell Updated! Removed the print comment and added the TODO reference as requested.

@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 13, 2026

thinking that we can simplify and report all of the options together, so lets:

  • completely remove the --extra_info flag (apologies for the detour with this)
  • move impratio to be reported on a new line below this line. we can compute impratio in the f-string using something like: 1.0 / np.square(m.opt.impratio_invsqrt)

thanks!

@thowell thowell changed the title feat: Add impratio and collision details to testspeed results feat: Add impratio to testspeed results Jan 13, 2026
@thowell thowell mentioned this pull request Jan 13, 2026
5 tasks
@zer-art
Copy link
Copy Markdown
Contributor Author

zer-art commented Jan 13, 2026

@thowell Done! I've removed the extra_info flag and updated the print statement to calculate impratio using the formula you provided.

Comment thread mujoco_warp/testspeed.py Outdated
Comment thread mujoco_warp/testspeed.py Outdated
Comment thread mujoco_warp/testspeed.py Outdated
f" solver: {solver} cone: {cone} iterations: {iterations} {ls_str}\n"
f" integrator: {integrator} graph_conditional: {m.opt.graph_conditional}"
f" integrator: {integrator} graph_conditional: {m.opt.graph_conditional}\n"
f" impratio: {1.0 / np.square(m.opt.impratio_invsqrt.numpy()[0]):g}"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what is the :g for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It stands for "general" format. I used it because impratio values can vary significantly in magnitude. :g automatically switches between fixed-point and scientific notation and removes insignificant trailing zeros, ensuring the output stays compact and readable.

@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 14, 2026

left some minor comments and a question, otherwise lgtm. after that all that is left to do is resolve any branch conflicts.

thanks!

@thowell thowell merged commit 2a6ba40 into google-deepmind:main Jan 16, 2026
9 checks passed
@thowell
Copy link
Copy Markdown
Collaborator

thowell commented Jan 16, 2026

thanks @zer-art!

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.

add more info to testspeed results

2 participants