-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Code review notes #79
Comments
Checklist for completing/addressing code review.
|
Hi @balisujohn, a couple of pointers mentioned in this issue I wanted to get feedback on.
|
How would you define a better representation for that? Is it important? |
A dataclass can be frozen.
Something more concise with the important characteristics of the dataset. Maybe something about the number of steps, obs/action spaces, or simply just the shapes of all the arrays stored inside. |
Hi, @shreyansjainn I agree with you that we at least directly use environment name. I don't think d4RL's naming style is a good choice, But let's consider futher of naming style. Though it may cause confusion if two versions mentioned in the name, but what if we consider data-names as a"leaf" of given environment. I mean, we could load dataset by using minari.load_dataset(env_name, data_name), for dataset we consider stroage structure like:
I think for long term thinking, definitely there will be different enviornment version with difeerent data versions, in that case only version would be more messy than using two version. Like "door-human-v2" and AndroitDoor, like "Hopper-v2" to "hopper-medium-v2", it's very inconviniennt. I think definitely most of d4rl datasets will be migrated to minari, and considering future data collection, using two-level naming style is the most intuitive solution and very easy to keep compatity. |
Writing this without fixes for now, things I'm noticing going through the library:
'door-cloned-v0', 'door-expert-v0', 'door-human-v0', ...
, at least reading it initially. One issue here is that the dataset names don't really tell you much unless you already know the context - we should probably mention that they're specifically for mujoco, probably specifically for some specific model - maybe we can list the name of the gymnasium env?minari.download_dataset("door-human-v0")
results in a pretty ugly/uncaught import error forgymnasium_robotics
. Since gymnasium-robotics isn't a necessary dependency, we should have a more elegant way of handling this.overwrite
orforce
argument to do that behavior, and by default it should just skip downloading it. It might be worthwhile to think if we want to do explicit downloads like this in the first place, but that's another thing.EpisodeData
is a namedtuple, which leads to the default representation being an enormous printout of all observations, rewards etc. This is not very useful, so we should probably make it a proper dataclass instead, and override the default representation.Stopping here for now, @rodrigodelazcano let me know if there's anything I'm missing anything for any of those points (like some additional reasons for doing X thing in a certain way that I'm unaware of). Otherwise we can make this into a to-do list of improvements
The text was updated successfully, but these errors were encountered: