Skip to content

ObservableAttribute #3925

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
May 15, 2020
Merged

ObservableAttribute #3925

merged 25 commits into from
May 15, 2020

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented May 4, 2020 •
edited
Loading

Proposed change(s)

Adds a new attribute, ObservableAttribute, which allows users to mark up Agent fields and properties to be turned into observations. Currently, the following data types are supported: int, float, bool, Vector2/3/4, Quaternion.

There is a non-zero (and possibly non-trivial) overhead for using these; they're recommended for getting started, but for production, users should use CollectObservations or a custom ISensor implementation.

Future work (not in this PR, links to jiras):
Benchmark
Caching
Support for Enums (?)
Convert some examples and retrain

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

https://mattwarren.org/2016/12/14/Why-is-Reflection-slow/
https://jira.unity3d.com/browse/MLA-11

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

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
chriselion commented May 5, 2020
View reviewed changes
surfnerd
surfnerd reviewed May 5, 2020
View reviewed changes
chriselion
chriselion commented May 13, 2020
View reviewed changes
Project/Assets/ML-Agents/Examples/FoodCollector/Scripts/FoodCollectorAgent.cs Outdated
@@ -39,15 +44,13 @@ public override void Initialize()
SetResetParameters();
}

public override void CollectObservations(VectorSensor sensor)
[Observable]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to either revert this scene change, or retrain the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted. Separate jira to retrain.

chriselion
chriselion commented May 13, 2020
View reviewed changes
com.unity.ml-agents/Runtime/Agent.cs Outdated
/// <summary>
/// Options for controlling how the Agent class is searched for <see cref="ObservableAttribute"/>s.
/// </summary>
public enum ObservableAttributeHandling
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 don't like any of the names here :/ Suggestions welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

ObservableAttributeOptions?

Copy link
Contributor

@surfnerd surfnerd May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if SensorAttribute makes it more consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to ObservableAttributeOptions

chriselion
chriselion commented May 13, 2020
View reviewed changes
com.unity.ml-agents/Runtime/Sensors/Reflection/ObservableAttribute.cs Outdated
m_NumStackedObservations = numStackedObservations;
}

internal static IEnumerable<(FieldInfo, ObservableAttribute)> GetObservableFields(object o, bool declaredOnly)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if these would be cleaner as a single IEnumerable<(MemberInfo, ObservableAttribute)> that returned both FieldInfos and PropertyInfos. Downside is that we'd have to try to cast before using in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might want to do this for caching, but it's OK for now.

chriselion
chriselion commented May 13, 2020
View reviewed changes
com.unity.ml-agents/Tests/Editor/Communicator/GrpcExtensionsTests.cs.meta
@@ -0,0 +1,11 @@
fileFormatVersion: 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this in a previous PR.

@chriselion chriselion marked this pull request as ready for review May 13, 2020 20:16
surfnerd
surfnerd reviewed May 13, 2020
View reviewed changes
chriselion
chriselion commented May 13, 2020
View reviewed changes
@chriselion chriselion changed the title ObservableAttribute proof-of-concept ObservableAttribute May 14, 2020
vincentpierre
vincentpierre reviewed May 14, 2020
View reviewed changes
com.unity.ml-agents/Editor/BehaviorParametersEditor.cs Outdated
@@ -98,6 +101,19 @@ void DisplayFailedModelChecks()
{
sensorComponents = behaviorParameters.GetComponents<SensorComponent>();
}

int observableSensorSizes = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Observable seems to be the name for the members marked with the Observable attribute. Maybe in this code it should have a mode descriptive name. ReflexionObservable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean specifically in this part of the code, or in general? We already have "Reflection" in the namespace for the attribute; I don't think we need it in the attribute name as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the name of this variable specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to observableAttributeSensorTotalSize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and added a few comments)

vincentpierre
vincentpierre approved these changes May 15, 2020
View reviewed changes
com.unity.ml-agents/Editor/AgentEditor.cs Outdated
@@ -22,6 +22,9 @@ public override void OnInspectorGUI()
new GUIContent("Max Step", "The per-agent maximum number of steps.")
);

var observableAttributeBehavior = serializedAgent.FindProperty("m_observableAttributeHandling");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the m_observableAttributeHandling belongs on the behavior parameters, not on the Agent inspector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I like that better.

@chriselion chriselion merged commit bb24b57 into master May 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the attribute-sensors-poc branch May 15, 2020 20:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@surfnerd surfnerd surfnerd approved these changes

@vincentpierre vincentpierre vincentpierre approved these changes

@mmattar mmattar Awaiting requested review from mmattar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chriselion @surfnerd @vincentpierre