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

[Proposal] Default metadata in BaseMujocoEnv #1059

Open
1 task done
spiglerg opened this issue May 20, 2024 · 2 comments
Open
1 task done

[Proposal] Default metadata in BaseMujocoEnv #1059

spiglerg opened this issue May 20, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@spiglerg
Copy link

spiglerg commented May 20, 2024

Proposal

The current code for BaseMujocoEnv requires the env metadata dictionary to have fixed, pre-specified values. While this may be useful for future API changes, it doesn't seem very useful at the moment.


    assert self.metadata["render_modes"] == [
        "human",
        "rgb_array",
        "depth_array",
    ], self.metadata["render_modes"]
    if "render_fps" in self.metadata:
        assert (
            int(np.round(1.0 / self.dt)) == self.metadata["render_fps"]
        ), f'Expected value: {int(np.round(1.0 / self.dt))}, Actual value: {self.metadata["render_fps"]}'

I propose to set the metadata attribute in BaseMujocoEnv to the current fixed values


self.metadata["render_modes"] = [
            "human",
            "rgb_array",
            "depth_array",
        ]
self.metadata["render_fps"] = int(np.round(1.0 / self.dt))

This will enable classes derived from MujocoEnv to overwrite the dictionary (if required, in the future), but it will not force them to write an explicit metadata dictionary if the default values suffice, reducing redundancy and making derived classes more compact and readable.

Motivation

No response

Pitch

No response

Alternatives

No response

Additional context

No response

Checklist

  • I have checked that there is no similar issue in the repo
@spiglerg spiglerg added the enhancement New feature or request label May 20, 2024
@Kallinteris-Andreas
Copy link
Collaborator

The current implementation is to enable definition of metadata["render_fps"] after MujocoEnv.__init__ (see how Ant_v4 and Ant_v5 define metadata for example), if your proposal does not break anything, feel free to make a PR

@spiglerg
Copy link
Author

Perfect, I have sent a pull request. The pull request avoids breaking the existing behavior by defining default metadata as class attribute, and then setting render_fps dynamically (as a function of self.dt) only if the key has not been overridden by a child class.

PR: #1099

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants