Add stacking to multi-dimensional and compressed observations #4476

dongruoping · 2020-09-11T20:57:35Z

Proposed change(s)

Add observations stacking to multi-dimensional and compressed observations.
This allows stacking for visual observations and other multi-dimensional observations.

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

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

com.unity.ml-agents/Runtime/Sensors/CameraSensorComponent.cs

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

com.unity.ml-agents/Runtime/Sensors/CameraSensor.cs

chriselion · 2020-09-21T18:24:54Z

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

+                }
+
+                // Construct compression mapping
+                var compressibleSensor = m_WrappedSensor as ICompressibleSensor;


You need to handle the case where the wrapped sensor isn't actually an ICompressibleSensor, just an ISensor. I think in that case, you should either leave the mapping empty, or the "identity" mapping (mapping[i] == i)

This is inside the else statement of checking m_WrappedSensor.GetCompressionType() which should have a compression type other than None. My intention is to make all sensors that can have compressed observation being ICompressibleSensor, so it should always be ICompressibleSensor here.
Or should I check and raise an exception here if the sensor has m_WrappedSensor.GetCompressionType() !=SensorCompressionType.None but its' not an ICompressibleSensor?

You shouldn't assume in general that m_WrappedSensor.GetCompressionType() !=SensorCompressionType.None means that it's an ICompressibleSensor. I think maybe a better name would change how we think about this - how about something like ISparseChannelSensor?

My intention is to make all sensors that can have compressed observation being ICompressibleSensor, so it should always be ICompressibleSensor here.

I actually think this might make things harder for backwards compatibility reasons. I'll start another thread on that.

This still needs to be addressed - if wrapping a non-ICompressibleSensor, assume an identity mapping.

chriselion · 2020-09-21T18:26:57Z

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

+                    offset = wrappedNumChannel * i;
+                    for (var j = 0; j < wrappedMapLength; j++)
+                    {
+                        m_CompressionMapping[j + wrappedMapLength * i] = wrappedMapping[j] > 0 ? wrappedMapping[j] + offset : 0;


Use -1 if the wrappedMapping is negative?

protobuf-definitions/proto/mlagents_envs/communicator_objects/observation.proto

chriselion · 2020-09-21T18:53:26Z

ml-agents-envs/mlagents_envs/tests/test_rpc_utils.py

@@ -85,6 +85,7 @@ def generate_compressed_data(in_array: np.ndarray) -> bytes:
 def generate_compressed_proto_obs(in_array: np.ndarray) -> ObservationProto:
    obs_proto = ObservationProto()
    obs_proto.compressed_data = generate_compressed_data(in_array)
+    obs_proto.mapping.extend(list(range(len(in_array))))


This needs some more unit tests. I think you should cover

1 channel, no mapping (old case)

3 channels, no mapping (old case)

3 channels, no mapping (old case)

1 channel, mapping

3 channel, mapping

3 channels with a non-trivial mapping (e.g. 4 channels and stacking=2)

Sure I'll add those.
But are we allowing compressed observation with 1 channel? I thought it should always be uncompressed.

compressed observation with 1 channel

Grayscale visual observations, and those can be stacked.

chriselion · 2020-09-21T21:42:46Z

You're also going to need to add some safeguards to make sure that different versions of C# and python can work OK with each other, or give an error early when then can't. To do this, you'll need to increase the communication protocol to 1.2.0 and add a flag in the Capabilities proto message - see #4462 for an example.

There's no problem using a new version of python with old C#, but if you have the latest C# using a non-trivial mapping and it's connect to an old version of the python code, you'll need to raise a warning and fall back to uncompressed observations, since the python code won't be able to decompress them correctly. Because of that, you might want consider not using the ICompressibleSensor for Camera and RenderTexture sensor, since it will be harder to detect when it's OK to use compression.

com.unity.ml-agents/Runtime/Sensors/RenderTextureSensorComponent.cs

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

com.unity.ml-agents/CHANGELOG.md

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

vincentpierre · 2020-09-28T16:54:52Z

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

+        }
+
+        public int[] ConstructStackedCompressedChannelMapping(ISensor wrappedSenesor)
+        {


This method does a lot of things. I think I know what it does, but you should add more comments here.

Added some comments. Let me know if anything still seems unclear.

vincentpierre · 2020-09-28T16:58:08Z

ml-agents-envs/mlagents_envs/rpc_utils.py

-    if actual_channels > expected_channels:
-        img = img[..., 0:expected_channels]
+    if mappings is not None and len(mappings) > 0:
+        image_arrays = np.concatenate(image_arrays, axis=2).transpose((2, 0, 1))


add a comment here. This method is getting pretty long, maybe adding some helper methods will help compartmentalize.

Can you do something like

mappings = list(range(expected_channels)) if expected_channels > 0 else [0, 0, 0]

so that the "old case" is handled by the new code?

Doesn't that get into the same performance debate for C# that a mapping has to be created every time?

Fair point :) I'm not sure if memory allocation/GC in python is better or worse than in C#, but it's at least tolerated in python.

Looks like you could use range(expected_channels) (although you'd have to make the mypy types a bit more generic) which should allocate less, and you could reuse (0, 0, 0)

I've split them into two functions now so it looks clear how it process with mapping or with expected_channels. If unifying them still looks better than this I can change it.

ml-agents-envs/mlagents_envs/rpc_utils.py

Co-authored-by: Vincent-Pierre BERGES <[email protected]>

com.unity.ml-agents/CHANGELOG.md

Co-authored-by: Chris Elion <[email protected]>

chriselion

Just a few small changes for what's there. But you're missing some unit test coverage on the new functionality. You can see what's covered here (this is from the latest CI run):
https://yamato-artifactviewer.prd.cds.internal.unity3d.com/10cef3c6-1a04-4bc2-9e8c-9ee059861eef%2Flogs%2Fupm-ci~%2Ftest-results%2Fisolation-3638200-com.unity.ml-agents%2FReport/index.htm

In particular, I think you need to:

Make a test that creates a dummy sensor that writes compressed observations, put a stacking sensor around that, and make sure the output bytes are expected. The dummy sensor doesn't need to output valid PNGs, you can just return {i} for the i'th step.
Make a test that has a non-multiple-of-3 number of channels, so that the -1 padding code will be tested. Again make a dummy sensor class that returns something like [3, 2, 1, 0] for the mapping and make sure the StackingSensor mapping is what you expect.
Add some test cases for IsTrivialMapping from GrpcExtensions

com.unity.ml-agents/CHANGELOG.md

chriselion · 2020-09-30T22:26:31Z

com.unity.ml-agents/Runtime/Communicator/GrpcExtensions.cs

+            {
+                if (mapping[i] != i)
+                {
+                    isIdentityMapping = false;


Return false here? Probably not worth optimizing, but might be clearer.

chriselion · 2020-09-30T22:34:13Z

com.unity.ml-agents/Runtime/Sensors/CameraSensorComponent.cs

@@ -75,6 +75,11 @@ public bool Grayscale
            set { m_Grayscale = value; }
        }

+        [HideInInspector, SerializeField]
+        [Range(1, 50)]
+        [Tooltip("Number of camera frames that will be stacked before being fed to the neural network.")]


Neat, I figured this would get ignored and you had to do it in the custom editor, but looks like it just works.

ml-agents-envs/mlagents_envs/rpc_utils.py

chriselion · 2020-09-30T22:58:47Z

ml-agents-envs/mlagents_envs/rpc_utils.py

-    if actual_channels > expected_channels:
-        img = img[..., 0:expected_channels]
+    if mappings is not None and len(mappings) > 0:
+        return process_images_mapping(image_arrays, mappings)


nit: leading _ for these functions?

chriselion · 2020-09-30T22:59:06Z

ml-agents-envs/mlagents_envs/rpc_utils.py


+# Old API without mapping provided. Use the first n channel, n=expected_channels.


move to docstring instead

Co-authored-by: Chris Elion <[email protected]>

dongruoping · 2020-10-02T18:10:32Z

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

@@ -85,11 +99,32 @@ public int Write(ObservationWriter writer)

            // Now write the saved observations (oldest first)
            var numWritten = 0;
-            for (var i = 0; i < m_NumStackedObservations; i++)
+            if (wrappedShape.Length == 1)


Found this bug that I didn't change the Write method properly for uncompressed obs and it's still stacking on the first dimension. Here changed it to the last dimension for 2D observations so that inference can work properly.

edit: this will not affect 1D case.

chriselion · 2020-10-05T22:16:01Z

com.unity.ml-agents/Runtime/Sensors/ISparseChannelSensor.cs

+namespace Unity.MLAgents.Sensors
+{
+    /// <summary>
+    /// Sensor interface for generating observations.


Can you update the description here? I think it's copied from ISensor

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs

chriselion · 2020-10-06T17:35:36Z

com.unity.ml-agents/Tests/Editor/Sensor/StackingSensorTests.cs

+            // Test mapping with dummy layers that should be dropped
+            var dummySensor = new Dummy3DSensor();
+            dummySensor.Shape = new int[] { 2, 2, 4 };
+            dummySensor.Mapping = new int[] { 0, 1, 2, 3, -1, -1 };


The wrapped sensor shouldn't be adding -1's to its mapping, right? I think we decided this was the responsibility of the StackingSensor. The assert below can stay the same, though.

Actually, I guess you should try both - once where -1 is in the wrapped sensor's mapping to make sure they're preserved, and another to make sure they're added if needed.

yes I agree both cases can happen, added test for that.

chriselion

Looks good!

Ruo-Ping Dong added 5 commits September 9, 2020 17:55

add visual stacking

b1d4c89

stack observations on the last dimemsion

d8612a0

add stacking to renderTextureComponent

8c5f4ea

fix shape

eadb6d0

add comments

d8a36e4

dongruoping requested a review from chriselion September 11, 2020 20:57

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/CameraSensorComponent.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 11, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Show resolved Hide resolved

Ruo-Ping Dong added 3 commits September 11, 2020 16:42

add comments

54a5280

small fixes

c7eb180

add compression mapping

4f122ed

dongruoping requested a review from chriselion September 18, 2020 18:50

chriselion reviewed Sep 21, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/CameraSensor.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 21, 2020

View reviewed changes

protobuf-definitions/proto/mlagents_envs/communicator_objects/observation.proto Outdated Show resolved Hide resolved

chriselion reviewed Sep 21, 2020

View reviewed changes

Ruo-Ping Dong added 3 commits September 21, 2020 12:18

Merge branch 'master' into develop-vis-stack

240d280

change mapping to 0-index

d24ae6d

Add docstring for mapping and change the name in proto

38bfb8b

Ruo-Ping Dong added 5 commits September 21, 2020 18:43

add tests

8b5f6db

update communicator version

2743658

update communicator version

e9f4d14

fix test

3b3e08d

revert unneeded changes

bb011fc

chriselion reviewed Sep 28, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/RenderTextureSensorComponent.cs Outdated Show resolved Hide resolved

chriselion reviewed Sep 28, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Outdated Show resolved Hide resolved

vincentpierre reviewed Sep 28, 2020

View reviewed changes

Ruo-Ping (Rachel) Dong and others added 5 commits September 28, 2020 20:50

Update com.unity.ml-agents/CHANGELOG.md

b5bb5dd

Co-authored-by: Vincent-Pierre BERGES <[email protected]>

refactor

75dc21b

add checks for mapping

70cf227

comment

1202030

fix test

bd19165

dongruoping marked this pull request as ready for review September 29, 2020 18:24

add c# tests

d240077

chriselion reviewed Sep 30, 2020

View reviewed changes

com.unity.ml-agents/CHANGELOG.md Outdated Show resolved Hide resolved

Update com.unity.ml-agents/CHANGELOG.md

a515dc9

Co-authored-by: Chris Elion <[email protected]>

chriselion reviewed Oct 1, 2020

View reviewed changes

Ruo-Ping (Rachel) Dong and others added 3 commits September 30, 2020 20:51

Update com.unity.ml-agents/CHANGELOG.md

ac2a8ca

Co-authored-by: Chris Elion <[email protected]>

add tests

64787b0

fix Write for uncompressed obs to stack in the last dimension

92b34b0

dongruoping commented Oct 2, 2020

View reviewed changes

Ruo-Ping Dong added 2 commits October 2, 2020 17:05

add tests

1af0c84

add files for tests

62ee54d

chriselion reviewed Oct 5, 2020

View reviewed changes

com.unity.ml-agents/Runtime/Sensors/StackingSensor.cs Show resolved Hide resolved

Ruo-Ping Dong added 2 commits October 5, 2020 22:10

fix bug

b38154c

fix typo

1a77ceb

chriselion reviewed Oct 6, 2020

View reviewed changes

add more test

89ca381

chriselion approved these changes Oct 6, 2020

View reviewed changes

minor comment formatting

8ef815e

dongruoping merged commit 9ce7a8e into master Oct 6, 2020

delete-merged-branch bot deleted the develop-vis-stack branch October 6, 2020 20:54

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2021


		# Old API without mapping provided. Use the first n channel, n=expected_channels.

Add stacking to multi-dimensional and compressed observations #4476

Add stacking to multi-dimensional and compressed observations #4476

Uh oh!

Conversation

dongruoping commented Sep 11, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

chriselion commented Sep 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

chriselion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongruoping Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongruoping commented Sep 11, 2020 •

edited

Loading

dongruoping Oct 2, 2020 •

edited

Loading