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

[Bug Fix] Small fixes for new mujoco release 3.0.0 #746

Merged

Conversation

rodrigodelazcano
Copy link
Member

@rodrigodelazcano rodrigodelazcano commented Oct 19, 2023

Description

  • Fix for mujoco rendering: The MjData member solver_iter has been renamed to solver_niter as specified in the changelog. We are also not using islands in our environments so we just output the first element of the list. Fixes MjData has no attribute solver_iter google-deepmind/mujoco#1107

  • Fix for swimmer xml: As specified in the 11 point of the changelog the collision argument has been removed from option.
    This PR makes the changes specified in the migration guide. The changelog provides the migration guide for "pair" that in the previous mujoco release documentation appears to be named as "predefined" such as in the swimmer xml (seems like a release note errata). I haven't compared reproducibility with the current Swimmer-v5 environment, but as specified in the docs mujoco only guaranties reproducibility for a fixed version https://mujoco.readthedocs.io/en/3.0.0/computation/index.html?highlight=reproducibility#reproducibility

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

rodrigodelazcano added 2 commits October 18, 2023 23:36
@rodrigodelazcano rodrigodelazcano changed the title [Bug Fix] Small mujoco rendering fix for new mujoco release 2.3.0 [Bug Fix] Small mujoco rendering fix for new mujoco release 3.0.0 Oct 19, 2023
@rodrigodelazcano rodrigodelazcano changed the title [Bug Fix] Small mujoco rendering fix for new mujoco release 3.0.0 [Bug Fix] Small fixes for new mujoco release 3.0.0 Oct 19, 2023
@rodrigodelazcano
Copy link
Member Author

This will also require a version bump of mujoco to >=3.0 right?

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

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

We should be careful with changing the models, that would require a lot of validation

@pseudo-rnd-thoughts
Copy link
Member

We should be careful with changing the models, that would require a lot of validation

Is one of you able to gather training results for the updated swimmer to compare to the old?

@Kallinteris-Andreas
Copy link
Collaborator

@pseudo-rnd-thoughts working on that

@rodrigodelazcano
Copy link
Member Author

rodrigodelazcano commented Oct 20, 2023

Thank you @Kallinteris-Andreas for testing. Just to add to the discussion, the xml changes don't affect the behavior/dynamics of the model. Seems like mujoco made this change because it was redundant to specify it this way. To explain how the changes don't modify the behavior:

  • collision="predefined" made mujoco check for collisions/contacts only with geoms that are specified as pairs (https://mujoco.readthedocs.io/en/2.3.7/XMLreference.html?highlight=pair#contact-pair). The Swimmer xml doesn't have any geom pairs so no collisions are computed at all during simulation. This allows for the agent to slide without friction through the floor.
  • mujoco==3.0.0 removes the collision="predefined" argument in favor of disabling non-pair dynamic collisions (determined via contype/conaffinity) by setting contype and conaffinity to 0 in all geoms of the model.

Training results should be identically hopefully.

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

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

I tested the new swimmer.xml with those versions of mujoco for 1mil steps each and have confirmed identical behavior

(validation repo: https://github.com/Kallinteris-Andreas/gym-mjc-v5-model-validation/tree/main/swimmer)

tested mujoco versions
"mujoco==2.1.5"
"mujoco==2.2.0"
"mujoco==2.2.1"
"mujoco==2.2.2"
"mujoco==2.3.0"
"mujoco==2.3.1"
"mujoco==2.3.2"
"mujoco==2.3.3"
"mujoco==2.3.5"
"mujoco==2.3.6"
"mujoco==2.3.7"
"mujoco-py==2.1.2.14" # note rendering was not identical, but that is unrelated to the model #690

@@ -611,7 +611,7 @@ def _create_overlay(self):

self.add_overlay(bottomleft, "FPS", "%d%s" % (1 / self._time_per_render, ""))
self.add_overlay(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible to maintain support for both pre 3.0 and post 3.0 mujoco (which is important for reproducibility), by doing this

if mujoco.__version__ => "3.0.0":
    self.add_overlay(
        bottomleft, "Solver iterations", str(self.data.solver_niter[0] + 1)
    )
elif mujoco.__version__ < "3.0.0":
    self.add_overlay(
        bottomleft, "Solver iterations", str(self.data.solver_iter + 1)
    )

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

So we are good to merge with no impact on the end users

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.

MjData has no attribute solver_iter
3 participants