Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VectorSensor and StackedSensor #2813

Merged
merged 21 commits into from
Nov 1, 2019
Merged

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Oct 28, 2019

Addresses https://jira.hq.unity3d.com/browse/MLA-231 and https://jira.hq.unity3d.com/browse/MLA-232

Adds VectorSensor and StackedSensor implementations, and uses them for Agent observations. Now all observations being sent to python and barracuda go through the ISensor interface.

The BrainParameters for number of vector observations and stacked observations are still used during Agent setup but are unused after this. They will be removed from the communication protocol in a subsequent PR.

@@ -92,6 +92,8 @@ public void Generate(TensorProxy tensorProxy, int batchSize, IEnumerable<Agent>
foreach (var agent in agents)
{
var info = agent.Info;
// TODO decide what to do here - should we have 1:1 sensor:tensor, or all sensors write
// consecutively to the same 1 tensor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be future oriented, I would say one sensor --> one tensorbut that will require changes to the trainer code...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote it to support many sensors to one tensor. We could revisit in the future, but this is pretty tightly coupled with both the trainer and the barracuda import.

UnitySDK/Assets/ML-Agents/Scripts/Sensor/ISensor.cs Outdated Show resolved Hide resolved
UnitySDK/Assets/ML-Agents/Scripts/Sensor/StackingSensor.cs Outdated Show resolved Hide resolved
UnitySDK/Assets/ML-Agents/Scripts/Sensor/VectorSensor.cs Outdated Show resolved Hide resolved

namespace MLAgents.Tests
{
public class EditModeTestInternalBrainTensorGenerator
{
static IEnumerable<Agent> GetFakeAgentInfos()
static IEnumerable<Agent> GetFakeAgents()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename since it returns Agents. This changed a fair amount in order to initialize the agents before returning

Copy link
Contributor

@surfnerd surfnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

/// <summary>
/// Allows sensors to write to both TensorProxy and float arrays/lists.
/// </summary>
public class WriteAdapter
Copy link
Contributor

@vincentpierre vincentpierre Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about how I feel about this write adapter. Is this going to be user facing ? Is this what we want the API to look like eventually or is this an intermediate form?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not user facing, unless they implement a custom ISensor, and even then, they only need to write to it, and even then they can use SensorBase to avoid it altogether.

I took this approach instead of trying to use ArraySegment (which might have had it's own problems) because writes to Barracuda.Tensor have some additional sideeffects (calls PrepareCacheForAccess and sets a dirty flag); I couldn't see another suitable way to abstract the writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to hide it or mark it as "don't use"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(talked offline, Vince is OK with this)

@chriselion chriselion changed the title WIP VectorSensor and StackedSensor VectorSensor and StackedSensor Oct 31, 2019
@chriselion chriselion marked this pull request as ready for review October 31, 2019 23:56
@chriselion chriselion merged commit 73368d4 into develop Nov 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the develop-vector-obs-sensors branch November 1, 2019 20:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants