From 5fd617d87f0435865c7e453fb7a697623a2f4cbe Mon Sep 17 00:00:00 2001 From: Chris Elion Date: Mon, 29 Mar 2021 14:28:22 -0700 Subject: [PATCH 1/3] culture-agnostic sorting for sensors and actuators --- .../Runtime/Actuators/ActuatorManager.cs | 10 ++-- com.unity.ml-agents/Runtime/Agent.cs | 2 +- .../Inference/BarracudaModelExtensions.cs | 7 ++- .../Runtime/Sensors/ISensor.cs | 13 +++++ .../Editor/Actuators/ActuatorManagerTests.cs | 28 +++++++++ .../Tests/Runtime/Sensor/SensorUtilTests.cs | 57 +++++++++++++++++++ .../Runtime/Sensor/SensorUtilTests.cs.meta | 3 + 7 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs create mode 100644 com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs.meta diff --git a/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs b/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs index e8914aeab0..230427c1b4 100644 --- a/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs +++ b/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs @@ -12,7 +12,7 @@ namespace Unity.MLAgents.Actuators internal class ActuatorManager : IList { // IActuators managed by this object. - IList m_Actuators; + List m_Actuators; // An implementation of IDiscreteActionMask that allows for writing to it based on an offset. ActuatorDiscreteActionMask m_DiscreteActionMask; @@ -95,7 +95,7 @@ internal void ReadyActuatorsForExecution(IList actuators, int numCont #endif // Sort the Actuators by name to ensure determinism - SortActuators(); + SortActuators(m_Actuators); var continuousActions = numContinuousActions == 0 ? ActionSegment.Empty : new ActionSegment(new float[numContinuousActions]); var discreteActions = numDiscreteBranches == 0 ? ActionSegment.Empty : new ActionSegment(new int[numDiscreteBranches]); @@ -317,11 +317,9 @@ public void ResetData() /// /// Sorts the s according to their value. /// - void SortActuators() + internal static void SortActuators(List actuators) { - ((List)m_Actuators).Sort((x, - y) => x.Name - .CompareTo(y.Name)); + actuators.Sort((x, y) => string.CompareOrdinal(x.Name, y.Name)); } /// diff --git a/com.unity.ml-agents/Runtime/Agent.cs b/com.unity.ml-agents/Runtime/Agent.cs index 985725d0d5..b50f6a82d3 100644 --- a/com.unity.ml-agents/Runtime/Agent.cs +++ b/com.unity.ml-agents/Runtime/Agent.cs @@ -989,7 +989,7 @@ internal void InitializeSensors() } // Sort the Sensors by name to ensure determinism - sensors.Sort((x, y) => x.GetName().CompareTo(y.GetName())); + SensorUtils.SortSensors(sensors); #if DEBUG // Make sure the names are actually unique diff --git a/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs b/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs index 5cb4d15c74..b53592308b 100644 --- a/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs +++ b/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Linq; using Unity.Barracuda; @@ -34,7 +35,7 @@ public static string[] GetInputNames(this Model model) names.Add(mem.input); } - names.Sort(); + names.Sort(StringComparer.Ordinal); return names.ToArray(); } @@ -87,7 +88,7 @@ public static IReadOnlyList GetInputTensors(this Model model) }); } - tensors.Sort((el1, el2) => el1.name.CompareTo(el2.name)); + tensors.Sort((el1, el2) => string.CompareOrdinal(el1.name, el2.name)); return tensors; } @@ -150,7 +151,7 @@ public static string[] GetOutputNames(this Model model) } } - names.Sort(); + names.Sort(StringComparer.Ordinal); return names.ToArray(); } diff --git a/com.unity.ml-agents/Runtime/Sensors/ISensor.cs b/com.unity.ml-agents/Runtime/Sensors/ISensor.cs index 8ca07ad830..fed2fc8506 100644 --- a/com.unity.ml-agents/Runtime/Sensors/ISensor.cs +++ b/com.unity.ml-agents/Runtime/Sensors/ISensor.cs @@ -1,3 +1,7 @@ +using System; +using System.Collections.Generic; +using System.Linq; + namespace Unity.MLAgents.Sensors { /// @@ -124,4 +128,13 @@ public static int ObservationSize(this ISensor sensor) return count; } } + + internal static class SensorUtils + { + internal static void SortSensors(List sensors) + { + // Use CompareOrdinal to ensure consistent sorting between different culture settings. + sensors.Sort((x, y) => string.CompareOrdinal(x.GetName(), y.GetName())); + } + } } diff --git a/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs b/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs index e095557ced..44cd639f16 100644 --- a/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs +++ b/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Globalization; using System.Linq; using NUnit.Framework; using Unity.MLAgents.Actuators; @@ -321,5 +323,31 @@ public void TestHeuristic() Assert.IsTrue(va2.m_HeuristicCalled); Assert.AreEqual(va2.m_DiscreteBufferSize, 4); } + + + /// + /// Test that sensors sort by name consistently across culture settings. + /// Example strings and cultures taken from + /// https://docs.microsoft.com/en-us/globalization/locale/sorting-and-string-comparison + /// + /// + [TestCase("da-DK")] + [TestCase("en-US")] + public void TestSortActuators(string culture) + { + List actuators = new List(); + var actuator0 = new TestActuator(ActionSpec.MakeContinuous(2), "Apple"); + var actuator1 = new TestActuator(ActionSpec.MakeContinuous(2), "Æble"); + actuators.Add(actuator0); + actuators.Add(actuator1); + + var originalCulture = CultureInfo.CurrentCulture; + CultureInfo.CurrentCulture = new CultureInfo(culture); + ActuatorManager.SortActuators(actuators); + CultureInfo.CurrentCulture = originalCulture; + + Assert.AreEqual(actuator0, actuators[0]); + Assert.AreEqual(actuator1, actuators[1]); + } } } diff --git a/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs new file mode 100644 index 0000000000..2c077172e2 --- /dev/null +++ b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs @@ -0,0 +1,57 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using NUnit.Framework; +using UnityEngine; +using Unity.MLAgents.Sensors; +using Unity.MLAgents.Utils.Tests; + +namespace Unity.MLAgents.Tests +{ + + [TestFixture] + public class SensorUtilTests + { + internal class TempCulture : IDisposable + { + private CultureInfo m_OriginalCulture; + + internal TempCulture(CultureInfo newCulture) + { + m_OriginalCulture = CultureInfo.CurrentCulture; + CultureInfo.CurrentCulture = newCulture; + } + + public void Dispose() + { + CultureInfo.CurrentCulture = m_OriginalCulture; + } + } + + /// + /// Test that sensors sort by name consistently across culture settings. + /// Example strings and cultures taken from + /// https://docs.microsoft.com/en-us/globalization/locale/sorting-and-string-comparison + /// + /// + [TestCase("da-DK")] + [TestCase("en-US")] + public void TestSortCulture(string culture) + { + List sensors = new List(); + var sensor0 = new TestSensor("Apple"); + var sensor1 = new TestSensor("Æble"); + sensors.Add(sensor0); + sensors.Add(sensor1); + + var originalCulture = CultureInfo.CurrentCulture; + CultureInfo.CurrentCulture = new CultureInfo(culture); + SensorUtils.SortSensors(sensors); + CultureInfo.CurrentCulture = originalCulture; + + Assert.AreEqual(sensor0, sensors[0]); + Assert.AreEqual(sensor1, sensors[1]); + } + + } +} diff --git a/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs.meta b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs.meta new file mode 100644 index 0000000000..c9e661ce9c --- /dev/null +++ b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: 929b34a718bc42c8aa75a3e1c8c11103 +timeCreated: 1617049947 \ No newline at end of file From 2bd6cd17f0e6ac768f8ec31ed860c5daf9759e88 Mon Sep 17 00:00:00 2001 From: Chris Elion Date: Mon, 29 Mar 2021 15:00:19 -0700 Subject: [PATCH 2/3] changelog --- com.unity.ml-agents/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/com.unity.ml-agents/CHANGELOG.md b/com.unity.ml-agents/CHANGELOG.md index 3a8cdaac99..5703bd5c3c 100755 --- a/com.unity.ml-agents/CHANGELOG.md +++ b/com.unity.ml-agents/CHANGELOG.md @@ -44,6 +44,9 @@ depend on the previous behavior, you can explicitly set the Agent's `InferenceDe ### Bug Fixes #### com.unity.ml-agents / com.unity.ml-agents.extensions (C#) +- Fixed a bug where sensors and actuators could get sorted inconsistently on different systems to different Culture +settings. Unfortunately, this may require retraining models if it changes the resulting order of the sensors +or actuators on your system. (#5194) #### ml-agents / ml-agents-envs / gym-unity (Python) From 92dc4827e8a8ff602d6aa13c6e35f66318ad3c4f Mon Sep 17 00:00:00 2001 From: Chris Elion Date: Tue, 30 Mar 2021 10:52:40 -0700 Subject: [PATCH 3/3] use InvariantCulture for sorting instead --- com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs | 2 +- .../Runtime/Inference/BarracudaModelExtensions.cs | 6 +++--- com.unity.ml-agents/Runtime/Sensors/ISensor.cs | 4 ++-- .../Tests/Editor/Actuators/ActuatorManagerTests.cs | 4 ++-- com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs b/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs index 230427c1b4..de0f239d68 100644 --- a/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs +++ b/com.unity.ml-agents/Runtime/Actuators/ActuatorManager.cs @@ -319,7 +319,7 @@ public void ResetData() /// internal static void SortActuators(List actuators) { - actuators.Sort((x, y) => string.CompareOrdinal(x.Name, y.Name)); + actuators.Sort((x, y) => string.Compare(x.Name, y.Name, StringComparison.InvariantCulture)); } /// diff --git a/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs b/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs index b53592308b..12ff03a12a 100644 --- a/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs +++ b/com.unity.ml-agents/Runtime/Inference/BarracudaModelExtensions.cs @@ -35,7 +35,7 @@ public static string[] GetInputNames(this Model model) names.Add(mem.input); } - names.Sort(StringComparer.Ordinal); + names.Sort(StringComparer.InvariantCulture); return names.ToArray(); } @@ -88,7 +88,7 @@ public static IReadOnlyList GetInputTensors(this Model model) }); } - tensors.Sort((el1, el2) => string.CompareOrdinal(el1.name, el2.name)); + tensors.Sort((el1, el2) => string.Compare(el1.name, el2.name, StringComparison.InvariantCulture)); return tensors; } @@ -151,7 +151,7 @@ public static string[] GetOutputNames(this Model model) } } - names.Sort(StringComparer.Ordinal); + names.Sort(StringComparer.InvariantCulture); return names.ToArray(); } diff --git a/com.unity.ml-agents/Runtime/Sensors/ISensor.cs b/com.unity.ml-agents/Runtime/Sensors/ISensor.cs index fed2fc8506..2038520f62 100644 --- a/com.unity.ml-agents/Runtime/Sensors/ISensor.cs +++ b/com.unity.ml-agents/Runtime/Sensors/ISensor.cs @@ -133,8 +133,8 @@ internal static class SensorUtils { internal static void SortSensors(List sensors) { - // Use CompareOrdinal to ensure consistent sorting between different culture settings. - sensors.Sort((x, y) => string.CompareOrdinal(x.GetName(), y.GetName())); + // Use InvariantCulture to ensure consistent sorting between different culture settings. + sensors.Sort((x, y) => string.Compare(x.GetName(), y.GetName(), StringComparison.InvariantCulture)); } } } diff --git a/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs b/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs index 44cd639f16..429d334828 100644 --- a/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs +++ b/com.unity.ml-agents/Tests/Editor/Actuators/ActuatorManagerTests.cs @@ -346,8 +346,8 @@ public void TestSortActuators(string culture) ActuatorManager.SortActuators(actuators); CultureInfo.CurrentCulture = originalCulture; - Assert.AreEqual(actuator0, actuators[0]); - Assert.AreEqual(actuator1, actuators[1]); + Assert.AreEqual(actuator1, actuators[0]); + Assert.AreEqual(actuator0, actuators[1]); } } } diff --git a/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs index 2c077172e2..0fe45897ba 100644 --- a/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs +++ b/com.unity.ml-agents/Tests/Runtime/Sensor/SensorUtilTests.cs @@ -49,8 +49,8 @@ public void TestSortCulture(string culture) SensorUtils.SortSensors(sensors); CultureInfo.CurrentCulture = originalCulture; - Assert.AreEqual(sensor0, sensors[0]); - Assert.AreEqual(sensor1, sensors[1]); + Assert.AreEqual(sensor1, sensors[0]); + Assert.AreEqual(sensor0, sensors[1]); } }