Allow visual and vector observations at the same time #3981

shakenes · 2020-05-16T09:12:19Z

Proposed change(s)

Allow vector observations also when visual observations are used. In this case, the array of vector observations are appended to the list of visual observations.
Compatibility should be maintained. If no visual observations are used, this PR changes nothing.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

The initial discussion including arguments took place here: #3960

Types of change(s)

Checklist

Added tests that prove my fix is effective or that my feature works
Updated the changelog (if applicable)
Updated the documentation (if applicable)
Updated the migration guide (if applicable)

Other comments

…ously

shakenes · 2020-05-18T09:56:38Z

Sorry for the mess. I forgot about the tests and just learned about pre-commit.

I think adjusting the test in that way is fine, because as discussed in the linked issue, vector observations should not be omitted when visual observations are also used. I added a test where both types of observations are used. In that case the observations are stored in a list with the last element containing the vectors observations.

vincentpierre

Looks good to me, a few minor comments.
Let me know if you want to address some of the comments or if you would rather merge this first.

vincentpierre · 2020-05-18T23:46:33Z

gym-unity/gym_unity/tests/test_gym.py

+    assert actions.shape[0] == 2
+    obs, rew, done, info = env.step(actions)
+    assert isinstance(obs[0], np.ndarray)
+    assert isinstance(obs[1], np.ndarray)


Can you do an assert on the shape of the vector_action ?

vincentpierre · 2020-05-18T23:54:44Z

gym-unity/gym_unity/envs/__init__.py

@@ -40,7 +40,7 @@ def __init__(
        """
        Environment initialization
        :param unity_env: The Unity BaseEnv to be wrapped in the gym. Will be closed when the UnityToGymWrapper closes.
-        :param use_visual: Whether to use visual observation or vector observation.
+        :param use_visual: Whether to use visual observations.


I think this is breaking the assumption that if use_visual is enabled, the observation will be a np.array and not a list of np.array.
I think this change is more suited for allow_multiple_visual_obs.
Maybe renaming allow_multiple_visual_obs to allow_multiple_obs and always return a list of (all visual and vector) observations would be better?

From my point of view your suggestion would be the better and cleaner solution. I tried to keep it as compatible as possible, but the changes one has to make to get his or her code working again would be rather simple. I'll add this to the PR.

Just realizing: What exactly should use_visual mean then? Does it mean there are one or more visual observations? That's would the name implies. However, that does not mean that the observation will be a numpy array.

I think it would make more sense to have three parameters here:

use_vector_only: Use only vector observations. Environments returns an np.array use_single_visual: Use a single visual observation. Environments returns an np.array allow_multiple_obs: Allows vector and multiple visual observations. Environment returns a list of observations with the last element containing the array of vector observations.

Let me know what you think.

That looks pretty nice. I wonder if an enum would be more appropriate in that case, a user could do use_vector_only=True, use_visual_only=True by mistake.

I think the simplest would be a single boolean : use_multiple_obs (default false)

If true, returns a list of np_array as observations

If false, will return a single np_array AND will return the first visual observation if any (over vector observation)

What do you think ?

That's probably even better. That way you know whether you get a list or an array. I can take care of that tomorrow and commit it here.

That would be awesome, thank you for your contributions !

vincentpierre · 2020-05-18T23:55:31Z

gym-unity/gym_unity/tests/test_gym.py

+def test_gym_wrapper_visual_and_vector(use_uint8):
+    mock_env = mock.MagicMock()
+    mock_spec = create_mock_group_spec(
+        number_visual_observations=1, vector_observation_space_size=3


Please add a test with multiple visual observations to test this line https://github.com/Unity-Technologies/ml-agents/pull/3981/files#diff-ae209b1396be959a453713c25c947969R196

Ok! Makes sense

…le_obs which allows visual and vector observations to be used simultaneously.

shakenes · 2020-05-20T07:24:39Z

Done. Let me know what you think.
There are some examples in the gym-unity Readme, that should be tested again. But I don't have the time to setup everything. Maybe someone can check on it who has everything installed already?

vincentpierre

I will do some more testing on my end but I like where the PR is right now.

I notice that we never implemented self.observation_shape for the case with multiple observations. I think we will need to implement a Tuple but I think this is out of scope of this PR.

I will try to have it merged into master or onto a develop branch if I want to make edits.

vincentpierre · 2020-05-20T17:22:44Z

gym-unity/gym_unity/envs/__init__.py

+            logger.warning(
+                "The environment contains visual and vector observations. "
+                "You must define allow_multiple_obs=True to receive them all. "
+                "Otherwise, please note that only the first visual observations will be provided int the observation. "


Suggested change

"Otherwise, please note that only the first visual observations will be provided int the observation. "

"Otherwise, please note that only the first visual observations will be provided in the observation. "

vincentpierre · 2020-05-20T17:31:49Z

gym-unity/gym_unity/envs/__init__.py

@@ -120,7 +129,7 @@ def __init__(
            high = np.array([1] * self.group_spec.action_shape)
            self._action_space = spaces.Box(-high, high, dtype=np.float32)
        high = np.array([np.inf] * self._get_vec_obs_size())
-        if self.use_visual:
+        if self._get_n_vis_obs() == 1:


This should be

Suggested change

if self._get_n_vis_obs() == 1:

if self._get_n_vis_obs() > 0 and not self. _allow_multiple_obs:

Since we return a single visual obs only if allow_multiple_obs=True

vincentpierre · 2020-05-20T17:48:37Z

I am sure some yamato tests will be failing on master because of lines like this in our CI.
I will make a new branch off master and merge this PR into this new branch and make edits there.

vincentpierre · 2020-05-20T18:07:58Z

I will make a new branch off master and merge this PR into this new branch and make edits there.

Correction : After reviewing our guidelines, I duplicated your branch and made an internal pull request for it. You will be marked as co-author on this PR and I will be able to do more tests and changes directly.

shakenes · 2020-05-20T18:38:08Z

Thanks for your help! I'm looking forward to having this feature released :)

vincentpierre · 2020-05-20T22:58:24Z

Merged #3998 into master

shakenes added 2 commits May 16, 2020 10:58

allow vector observations also when using visual observations

dcc5145

update changelog

7478746

shakenes changed the title ~~Allow both obs~~ Allow visual and vector observations at the same time May 16, 2020

shakenes and others added 6 commits May 16, 2020 11:14

Update CHANGELOG.md

bbcf3c0

Update __init__.py

d1ea4b6

remove trailing whitespace

359cd71

Fix test case where visual and vector observations are used simultane…

7ce24fe

…ously

fix formatting

95be889

add test for visual and vector observations

772c171

awjuliani requested a review from vincentpierre May 18, 2020 17:27

vincentpierre reviewed May 18, 2020

View reviewed changes

vincentpierre assigned shakenes and vincentpierre May 18, 2020

shakenes added 3 commits May 19, 2020 07:35

Assert vector action shape

8169f5f

Fix test environment to return multiple visual observations

a31d700

use_visual and allow_multiple_visual_obs are replaced by allow_multip…

957b6fd

…le_obs which allows visual and vector observations to be used simultaneously.

shakenes requested a review from vincentpierre May 20, 2020 07:25

vincentpierre reviewed May 20, 2020

View reviewed changes

vincentpierre changed the base branch from master to shakenes-allow_both_obs May 20, 2020 17:50

vincentpierre approved these changes May 20, 2020

View reviewed changes

vincentpierre mentioned this pull request May 20, 2020

External contribution : Allow visual and vector observations at the same time #3998

Merged

10 tasks

vincentpierre closed this May 20, 2020

shakenes deleted the allow_both_obs branch May 21, 2020 17:48

github-actions bot locked as resolved and limited conversation to collaborators May 21, 2021

	"Otherwise, please note that only the first visual observations will be provided int the observation. "
	"Otherwise, please note that only the first visual observations will be provided in the observation. "

	if self._get_n_vis_obs() == 1:
	if self._get_n_vis_obs() > 0 and not self. _allow_multiple_obs:

Allow visual and vector observations at the same time #3981

Allow visual and vector observations at the same time #3981

Uh oh!

Conversation

shakenes commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change(s)

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Types of change(s)

Checklist

Other comments

Uh oh!

shakenes commented May 18, 2020

Uh oh!

vincentpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shakenes commented May 20, 2020

Uh oh!

vincentpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentpierre commented May 20, 2020

Uh oh!

vincentpierre commented May 20, 2020

Uh oh!

shakenes commented May 20, 2020

Uh oh!

vincentpierre commented May 20, 2020

Uh oh!

Uh oh!

shakenes commented May 16, 2020 •

edited

Loading