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

Use eval_max_episode lengths in evalutation #1853

Closed
wants to merge 1 commit into from

Conversation

avnishn
Copy link
Member

@avnishn avnishn commented Aug 4, 2020

This commit fixes a bug where when obtaining
eval trajectories with sac, the max_eval_episode_length
isn't actually used by the algorithm or anywhere.

This commit fixes a bug where when obtaining
eval trajectories with sac, the max_eval_episode_length
isn't actually used by the algorithm or anywhere.
@avnishn avnishn requested a review from a team as a code owner August 4, 2020 23:18
@avnishn avnishn requested review from ahtsan and removed request for a team August 4, 2020 23:18
@mergify mergify bot requested review from a team, AiRuiChen and ziyiwu9494 and removed request for a team August 4, 2020 23:18
@avnishn avnishn requested review from krzentner and removed request for AiRuiChen and ziyiwu9494 August 4, 2020 23:18
@mergify mergify bot requested review from a team, haydenshively and AiRuiChen and removed request for a team August 4, 2020 23:18
@avnishn avnishn requested review from maliesa96 and removed request for haydenshively, AiRuiChen and a team August 4, 2020 23:18
@mergify mergify bot requested review from a team and yeukfu and removed request for a team August 4, 2020 23:18
@avnishn avnishn removed the request for review from yeukfu August 4, 2020 23:18
@avnishn avnishn added bug Something isn't working backport-to-2020.06 Backport this PR to release-2020.06 ready-to-merge labels Aug 4, 2020
@@ -45,7 +45,7 @@ class SAC(RLAlgorithm):
environment that the agent is being trained in. Usually accessable
by calling env.spec.
max_episode_length (int): Max path length of the algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

this description doesn't make much sense -- can you make it longer?

@@ -94,7 +94,7 @@ def __init__(
replay_buffer,
*, # Everything after this is numbers.
max_episode_length,
max_eval_path_length=None,
max_eval_episode_length=None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
max_eval_episode_length=None,
max_episode_length_eval=None,

Copy link
Member

Choose a reason for hiding this comment

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

For both of these, please explain what happens if the environment has a max_episode_length which is less than these numbers

@@ -94,7 +94,7 @@ def __init__(
replay_buffer,
*, # Everything after this is numbers.
max_episode_length,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
max_episode_length,
max_episode_length_explore,

Copy link
Member

Choose a reason for hiding this comment

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

if i don't set it, does it use the max_episode_length from the environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it is default 1000. I had done this when we were only doing apples to apples comparisons with the paper. A less confusing behavior would be do use max_episode_length_explore

Copy link
Member

Choose a reason for hiding this comment

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

i agree with that as long as max_episode_length_explore defaults to env.spec.max_episode_length

In general I think people shouldn't interact with these unless they have very specific needs (e.g. your baseline comparisons), e.g. this wouldn't appear in an example.

@avnishn avnishn linked an issue Aug 16, 2020 that may be closed by this pull request
@avnishn avnishn closed this Aug 16, 2020
@ryanjulian ryanjulian deleted the use_eval_path_length_SAC branch September 14, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-2020.06 Backport this PR to release-2020.06 bug Something isn't working ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MTSAC Max Path Length Incorrect
2 participants