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

Refactor parse_args() #118

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Refactor parse_args() #118

merged 2 commits into from
Feb 22, 2022

Conversation

vwxyzjn
Copy link
Owner

@vwxyzjn vwxyzjn commented Feb 21, 2022

This PR refactors a few arguments in parse_args():

  1. This PR moves the gym_id, learning_rate, total_timesteps move down to algorithm-specific arguments. Currently when doing a file diff between c51_atari.py and ppo_continuous_action.py, we have the following screenshot:
    image
    However, it's more desirable to all of the differences in one place, as shown below:
    image

  2. This PR also renames the gym-id to env-id. The reason is that as we adopt different environments, some of them don't necessarily have a gym-id but they have an environment name. For example, procgen has the env-id but not gym-id.

Curious about your thoughts @yooceii @dosssman on whether this is necessary.

@gitpod-io
Copy link

gitpod-io bot commented Feb 21, 2022

Copy link
Collaborator

@dosssman dosssman left a comment

Choose a reason for hiding this comment

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

I concur on the gym-id to env-id change, as the former is more general indeed.

For the position of the arguments in the argparser list, since some algorithms are kind of made for speicfic tasks (SAC -> Bulet / Mujoco), it does make sense to put it in the algorithms specific arguments. learning-rate too, because in SAC for example, there are actually 2 learning, so it would make the parameterization cleaner.

While total-timesteps is also general, I guess different algorithms training for different amount of steps could be possible (for example Atari's required 10M + vs Bullet's 1M) can be used as justification for this change.

In any case, it is all good on my side.

@vercel
Copy link

vercel bot commented Feb 22, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vwxyzjn/cleanrl/AAxRK8ytYUqJoht2izi8wtmoQzwA
✅ Preview: https://cleanrl-git-refactor-arg-vwxyzjn.vercel.app

@vwxyzjn
Copy link
Owner Author

vwxyzjn commented Feb 22, 2022

The matplotlib utilities might break but that's ok since we are re-building Open RL Benchmark anyway #115.

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.

None yet

2 participants