Fix a typo to allow evaluating algos deterministically

[maciejwolczyk]

When running some experiments with SAC I have discovered that my algorithm does not act deterministically during the evaluation (i.e. the action is sampled from the policy distribution instead of taking the mean/mode of the distribution). The code for obtaining evaluation samples uses the rollout function from sampler.utils with the argument deterministic=True.

The rollout function is then supposed to look into agent_info dictionary and use the mean value stored there. Unfortunately, currently in the code it looks into the agent_infos (with s at the end), which is a list containing agent_info dictionaries and as such obviously does not contain the mean key. This means that the stochastic, sampled action is used instead. My pull request solves this issue by fixing the typo.

Technical sidenote - maybe there should be an exception raised if deterministic=True and there is no mean key in the dict?

[krzentner]

Oh, I thought we had a test case for this code path. I guess not.

Honestly, looking for mean in the first place is kinda a hack. It only really makes sense for gaussian policies. I've wondered for some time if we should add a argument to get_action/get_actions that makes the actions deterministic.

[avnishn]

Thanks for catching this!

[avnishn]

We should probably add a corresponding test for this bug somewhere in this file:

https://github.com/rlworkgroup/garage/blob/master/tests/garage/sampler/test_utils.py

[ryanjulian]

@Mergifyio rebase

[mergify]

Command rebase: success

Branch has been successfully rebased

[ryanjulian]

@maciejwolczyk thanks so much for the PR!

Can you add a test which would have detected this bug (as suggested by @avnishn )?

After that, I think it's ready to merge.

[maciejwolczyk]

I'm glad I could help and sorry for the delay!

Is this test okay? I've run the test on the old code with the typo and it does fail, and of course it should pass after the fix.

[ryanjulian]

this test looks good to me! please just make the CI pass and we will merge it :)

thank you!

[ryanjulian]

@Mergifyio rebase

[mergify]

Command rebase: success

Branch has been successfully rebased

[ryanjulian]

@Mergifyio rebase

[mergify]

Command rebase: success

[ryanjulian]

@Mergifyio rebase

[mergify]

Command rebase: success

[ryanjulian]

@maciejwolczyk it looks like the CI is failing due to a formatting issue. Can you correct it and re-upload?

Note that this should be handled for you if you setup pre-commit, as detailed in CONTRIBUTING.md

[maciejwolczyk]

That's a bit weird, earlier I've checked the commits with the pre-commit hooks enabled and also the CI passed when it was ran for the first time... I guess maybe the failure now is because the pre-commit script changed recently?

I've pushed a commit which passes the pre-commit scripts. It was actually detecting a formatting error in lines that I haven't touched, so I guess it just checks the whole file. If you run yapf --recursive . in the repository dir then it finds some more formatting inconsistencies, but I think a separate pull request would be more appropriate for fixing that.

Anyway, I hope it will pass now.

[maciejwolczyk]

It's failing now, but on a later stage (normal tests) and the failing test is test_time_limit_env for the BulletEnv. I'm not sure if this issue is connected with the current pull request, I've checked that some of the CI for different PRs fail on this test as well.

[AiRuiChen]

It's failing now, but on a later stage (normal tests) and the failing test is test_time_limit_env for the BulletEnv. I'm not sure if this issue is connected with the current pull request, I've checked that some of the CI for different PRs fail on this test as well.

Hi, just sent #1710 to fix this failing test. Once this is merged please try rebasing the latest master branch and check CI again.

[maciejwolczyk]

@Mergifyio rebase

[mergify]

@maciejwolczyk is not allowed to run commands

[ryanjulian]

@Mergifyio rebase

[mergify]

Command rebase: success

[ryanjulian]

@Mergifyio backport release-2020.06

[ryanjulian]

@Mergifyio backport release-2019.10

[mergify]

Command backport release-2020.06: failure

No backport have been created

• Backport to branch release-2020.06 failed

Cherry-pick of f8377f5 has failed:

On branch mergify/bp/release-2020.06/pr-1617
Your branch is up to date with 'origin/release-2020.06'.

You are currently cherry-picking commit f8377f5f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   src/garage/sampler/utils.py
	modified:   tests/garage/sampler/test_utils.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   tests/fixtures/policies/dummy_policy.py

[mergify]

Command backport release-2019.10: failure

No backport have been created

• Backport to branch release-2019.10 failed

Cherry-pick of f8377f5 has failed:

On branch mergify/bp/release-2019.10/pr-1617
Your branch is up to date with 'origin/release-2019.10'.

You are currently cherry-picking commit f8377f5f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   tests/garage/sampler/test_utils.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   src/garage/sampler/utils.py
	both modified:   tests/fixtures/policies/dummy_policy.py