-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
WIP Made initial changes to enable dimension properties and added attention module #4763
Conversation
com.unity.ml-agents/Runtime/Sensors/IDimensionPropertiesSensor.cs
Outdated
Show resolved
Hide resolved
{ | ||
for (int i = 0; i < m_ObsSize * m_MaxNumObs; i++) | ||
{ | ||
writer[i] = m_ObservationBuffer[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is implicitly zero padded, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the assumption is that if all the floats in an obs are 0, then the observation is padded. This padding will be ignored by the trainers, but still fed as input during inference / training.
We can pad with another value, but I chose 0.
if (dimensionPropertySensor != null) | ||
{ | ||
var dimensionProperties = dimensionPropertySensor.GetDimensionProperties(); | ||
int[] intDimensionProperties = new int[dimensionProperties.Length]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you just loop and call observationProto.DimensionProperties.Add()
?
com.unity.ml-agents/Runtime/Sensors/IDimensionPropertiesSensor.cs
Outdated
Show resolved
Hide resolved
com.unity.ml-agents/Runtime/Sensors/IDimensionPropertiesSensor.cs
Outdated
Show resolved
Hide resolved
None = 1, | ||
|
||
/// <summary> | ||
/// Means it is possible to do a convolution in this dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it will be better by saying something like "suitable/meaningful to apply a convolution".
(It's possible to do a conv anyways.)
Co-authored-by: Ruo-Ping Dong <ruoping.dong@unity3d.com>
Co-authored-by: Ruo-Ping Dong <ruoping.dong@unity3d.com>
""" | ||
|
||
observation_shapes: List[Tuple] | ||
sensor_spec: List[SensorSpec] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sensor_spec: List[SensorSpec] | |
sensor_specs: List[SensorSpec] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, will change throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other issues you see with naming or design ?
@@ -129,7 +129,7 @@ def _warn_csharp_base_capabilities( | |||
if not caps.baseRLCapabilities: | |||
logger.warning( | |||
"WARNING: The Unity process is not running with the expected base Reinforcement Learning" | |||
" capabilities. Please be sure upgrade the Unity Package to a version that is compatible with this " | |||
" capabilities. Please be sure upgrade the Unity Package sensor_specsn that is compatible with this " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have a problem with my IDE, this should not have changed
@@ -36,7 +36,7 @@ def test_basic_in_registry(): | |||
for worker_id in range(2): | |||
assert BASIC_ID in registry | |||
env = registry[BASIC_ID].make( | |||
base_port=6005, worker_id=worker_id, no_graphics=True | |||
base_port=6002, worker_id=worker_id, no_graphics=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking if this change was intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super useful PR, LGTM
Proposed change(s)
Implementation according to this design : https://docs.google.com/document/d/1EvgrFjICvz9o3ymAVnR1O7nNxeH-4UknahQTGcjG3ww/edit#
When trying to train with a BufferSensor, the user will see the error message :
This Pr is just setting the ground work for allowing variable size observation (no trainer changes)
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments