[MLA-1634] Add ObservationSpec and update ISensor interfaces by chriselion · Pull Request #5127 · Unity-Technologies/ml-agents

chriselion · 2021-03-16T18:13:39Z

Proposed change(s)

Add an InplaceArray implementation for "blittable" arrays with a max size of 4. This is used for ObservationSpec shapes and dimension properties.
Add an ObservationSpec struct and helper methods.
Change ISensor interface to return ObservationSpec instead of just a shape array.
Remove IDimensionPropertiesSensor and ITypedSensor interfaces, use information from ObservationSpec instead.

Note that this does not introduce CompressionSpec or remove ISparseChannelSensor yet; this was getting big, so I'm saving that for a subsequent PR.

I haven't updated the docs or migration guide yet, will do that soon. Test coverage is done though.

Follow-Ups in subsequent PRs

Convert SensorComponent too
Introduce CompressionSpce and remove ISparseChannelSensor

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

https://jira.unity3d.com/browse/MLA-1634
Alternative to #5076
(Unity internal) design proposal

Types of change(s)

New feature
Code refactor
Breaking change

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

chriselion · 2021-03-16T18:14:49Z

    "m_Path": "ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json",
    "m_Dictionary": {
-        "m_DictionaryValues": []
+        "m_DictionaryValues": [


Not sure what to do with this. It got added in 7718dfe#diff-41dd07c8a0392e1f6c2a7ba1277f3426cdc92aa8258e563912c39db66800ade0 but wasn't computing coverage on the assemblies we care about.

yeah, I think it's just for local runs of code coverage

chriselion · 2021-03-16T18:15:46Z


-        /// <summary>Gets the shape of the grid observation</summary>
-        /// <returns>integer array shape of the grid observation</returns>
-        public int[] GetFloatObservationShape()


I think this was unused.

chriselion · 2021-03-16T18:16:28Z

        {
-            m_Shape = new[] { GridNumSideX, GridNumSideZ, ObservationPerCell };
-            return m_Shape;
+            var shape = m_ObservationSpec.Shape;


This is the SensorComponent override, not sure if we should change that to return a spec too.

I would lean toward yes

OK, I'm going to punt that to a separate PR.

chriselion · 2021-03-16T18:17:43Z

        /// <summary>
        /// Set whether or not the action index for the given branch is allowed.
        /// </summary>
+        /// <remarks>


Not from here, but was broken in staging.

chriselion · 2021-03-16T18:20:50Z

+    /// This is a value type that does not allocate any additional memory.
+    /// </summary>
+    /// <remarks>
+    /// This does not implement any interfaces such as IList, in order to avoid any accidental boxing allocations.


Small tradeoff between ergonomics and efficiency. It's small either way, so I'm willing to change this.

chriselion · 2021-03-16T18:21:46Z

+        /// <param name="lhs"></param>
+        /// <param name="rhs"></param>
+        /// <returns>Whether the arrays are equivalent.</returns>
+        public static bool operator==(InplaceArray<T> lhs, InplaceArray<T> rhs)


What's the recommended way to do operator== and Equals() for a value type? Should the implementation be in Equals() instead?

chriselion · 2021-03-16T18:22:21Z

+        /// <returns></returns>
+        public override int GetHashCode()
+        {
+            return Tuple.Create(m_Elem0, m_Elem1, m_Elem2, m_Elem3, Length).GetHashCode();


Not currently used anywhere, but I had to add this if I overrode Equals().

chriselion · 2021-03-16T18:23:56Z

+    /// <summary>
+    /// The ObservationType enum of the Sensor.
+    /// </summary>
+    public enum ObservationType


These were made public, but didn't make any other changes to them. We'll remove Reward and Message, and rename Goal later.

chriselion · 2021-03-16T18:31:42Z

+        /// For example, a sensor that observes the velocity of a rigid body (in 3D) would use [3].
+        /// A sensor that returns an RGB image would use [Height, Width, 3].
+        /// </summary>
+        public InplaceArray<int> Shape;


Open to discussion on the visibility for these fields, and also the constructor. I think the static helpers (Vector, VariableLength, Visual) are good for all the purposes that we currently support in the trainer, and a user could potentially implement their own trainer that supported different combos of properties (e.g. convolutional 1D or 3D).

I think we want to expose the ObservationSpec() constructor, and with that it's flexible enough for users to customize their own and not necessary to expose the fields

We check in the constructor that the length of shape and dimensionProperties match. Users can potentially set shape and dimensionProperties with different length if we expose the fields.

OK, you convinced me. I'll make the fields internal and add public get properties.

chriselion · 2021-03-16T18:33:12Z

-                    {
-                        Debug.Assert(cachedShape[j] == sensorShape[j], "Sensor sizes must match.");
-                    }
+                    var cachedSpec = m_SensorShapes[i];


This got a little simpler (and probably faster)

…ationSpec

surfnerd · 2021-03-17T23:47:36Z

@@ -1,9 +1,10 @@
 fileFormatVersion: 2


thank you, i saw this before and it was annoying me

Oh, it was accidental (but also annoying me).

vincentpierre

I like what I see

vincentpierre · 2021-03-17T23:52:09Z

+        /// </summary>
+        /// <param name="length"></param>
+        /// <returns></returns>
+        public static ObservationSpec Vector(int length)


For Vector and Visual, add ObservationType to the static constructors.

but make them default to Default? Sure, that sounds reasonable.

@vincentpierre Any reason to not add that to VariableLength also?

The goal conditioning trainer will not work with VariableLength obs. We can add it though, but the trainer will raise an error or a warning when this happens.

Good call, removed the variable length.

vincentpierre · 2021-03-17T23:54:16Z

+    /// This does not implement any interfaces such as IList, in order to avoid any accidental boxing allocations.
+    /// </remarks>
+    /// <typeparam name="T"></typeparam>
+    public struct InplaceArray<T> where T : struct


I would add a

public struct ObservationShape : InplaceArray<int> { }

And same for DimensionProperties. Thoughts? Maybe overkill

I think the constructors wouldn't work without adding them to ObservationShape too (plus DimensionProperties), which I'd rather not do.

And using ObservationShape = InplaceArray<int>; is only scoped for the file it's declared in, so wouldn't help any calling code.

surfnerd · 2021-03-18T00:01:31Z

            {
                dimInfos[i].Size = shape[i];
-                dimInfos[i].Flags = dimProps != null ? (int)dimProps[i] : 0;
+                dimInfos[i].Flags = (int)dimProps[i];


dongruoping · 2021-03-18T00:20:43Z

+    /// This does not implement any interfaces such as IList, in order to avoid any accidental boxing allocations.
+    /// </remarks>
+    /// <typeparam name="T"></typeparam>
+    public struct InplaceArray<T> where T : struct


Is there any plans of adding methods for converting between this and barracuda tensorshape?

Maybe we could do it as an extension method somewhere. Where would it be used?

Let's save this for another PR - don't think it'll affect the public API

Chris Elion added 12 commits March 10, 2021 14:24

ObservationSpec proposal

492d0b4

WIP obs spec

43abc71

use ObservationSpec everywhere

40f78a9

InplaceArray for shape

e917a8f

spec and inplacearray cleanup

e436125

Merge remote-tracking branch 'origin/v2-staging' into csharp-obs-spec

aa751a1

remove IDimensionPropertiesSensor.cs

e8403af

remove ITypedSensor

c79b9b4

InplaceArray and ObsSpec test

1cf71bf

more coverage (and update coverage settings)

7fe748a

remove compressionspec for now, docstrings

89018a4

more docstrings

30b6115

chriselion commented Mar 16, 2021

View reviewed changes

Comment thread com.unity.ml-agents/Runtime/Sensors/ObservationSpec.cs Outdated

chriselion commented Mar 16, 2021

View reviewed changes

Chris Elion added 3 commits March 16, 2021 11:37

cleanup

2886994

Merge remote-tracking branch 'origin/v2-staging' into MLA-1634-Observ…

ea66b15

…ationSpec

format

71266ad

chriselion requested review from dongruoping, surfnerd and vincentpierre March 16, 2021 18:57

chriselion commented Mar 16, 2021

View reviewed changes

Comment thread com.unity.ml-agents/Tests/Editor/InplaceArrayTests.cs Outdated

Chris Elion added 2 commits March 16, 2021 13:01

remove accidental using

44e2e61

fix test exception

8d1d973

chriselion mentioned this pull request Mar 16, 2021

ObservationSpec proposal #5076

Closed

10 tasks

Chris Elion added 4 commits March 17, 2021 11:04

changelog, migration, fix name in obs

ff254a8

Make ObsSpec.Shape and DimProps internal, add get props

8bfd913

ObsType internal too, NumDimensions to Rank

dbf93c9

Merge remote-tracking branch 'origin/v2-staging' into MLA-1634-Observ…

875e03c

…ationSpec

surfnerd reviewed Mar 17, 2021

View reviewed changes

vincentpierre approved these changes Mar 17, 2021

View reviewed changes

surfnerd reviewed Mar 18, 2021

View reviewed changes

dongruoping reviewed Mar 18, 2021

View reviewed changes

dongruoping approved these changes Mar 18, 2021

View reviewed changes

Chris Elion added 2 commits March 17, 2021 18:19

optional ObsType in ObsSpec utils, clean up InplaceArray

0dabf84

don't allow obs type for variable length

3078dfd

chriselion merged commit a0b1578 into v2-staging Mar 18, 2021

delete-merged-branch Bot deleted the MLA-1634-ObservationSpec branch March 18, 2021 18:13

surfnerd pushed a commit that referenced this pull request Mar 18, 2021

[MLA-1634] Add ObservationSpec and update ISensor interfaces (#5127)

e80c26b

surfnerd pushed a commit that referenced this pull request Mar 18, 2021

[MLA-1634] Add ObservationSpec and update ISensor interfaces (#5127)

ec36efe

surfnerd pushed a commit that referenced this pull request Mar 18, 2021

[MLA-1634] Add ObservationSpec and update ISensor interfaces (#5127)

0697de8

surfnerd pushed a commit that referenced this pull request Mar 18, 2021

[MLA-1634] Add ObservationSpec and update ISensor interfaces (#5127)

5069f5c

surfnerd pushed a commit that referenced this pull request Mar 18, 2021

[MLA-1634] Add ObservationSpec and update ISensor interfaces (#5127)

540595a

github-actions Bot locked as resolved and limited conversation to collaborators Mar 18, 2022

Conversation

chriselion commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change(s)

Follow-Ups in subsequent PRs

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

Types of change(s)

Checklist

Other comments

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chriselion commented Mar 16, 2021 •

edited

Loading