-
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
[MLA-345] float visual observations #3148
Conversation
- target: {fileID: 4124767863011510, guid: 5c2bd19e4bbda4991b74387ca5d28156, type: 2} | ||
- target: {fileID: 1488387672112076, guid: 5c2bd19e4bbda4991b74387ca5d28156, type: 3} | ||
propertyPath: m_Name | ||
value: FloatAgent |
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.
Change one of the prefab instances to have a different name and compression type.
def test_process_pixels(): | ||
in_array = np.random.rand(128, 128, 3) | ||
in_array = np.random.rand(128, 64, 3) |
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.
Made these non-square. Fortunately no errors.
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.
Good idea
@@ -119,27 +118,6 @@ def __init__( | |||
self.agents = agents | |||
self.action_masks = action_mask | |||
|
|||
@staticmethod | |||
@timed | |||
def process_pixels(image_bytes: bytes, gray_scale: bool) -> np.ndarray: |
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.
This was identical to the rpc_utils version. It now calls the new rpc_utils method.
|
||
namespace MLAgents.Tests | ||
{ | ||
public class Float2DSensor : ISensor |
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.
This is probably usable on its own, but I can't think of a good usecase in our current examples. It would be cool to use for occupancy maps (see https://www.gdcvault.com/play/1267/(307)-Beyond-Behavior-An-Introduction starting around 13:00)
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.
Maybe we can move it out of tests and to a fully-supported component when we make a hide-and-seek demo.
def test_process_pixels(): | ||
in_array = np.random.rand(128, 128, 3) | ||
in_array = np.random.rand(128, 64, 3) |
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.
Good idea
); | ||
|
||
// Math copied from TensorShape.Index(). Note that m_Batch should always be 0 | ||
var index = m_Batch * height * width * channels + h * width * channels + w * channels + ch; |
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 reason we don't use TensorShape
instead of int[]
?
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 would say it's a bit less user-friendly than int[]
because you have to pass 1 to the batch size for our usage. It would also prevent us from using higher-dimensional observations (although I guess we wouldn't be able to run inference on that either). Plus I didn't know about it at the time :)
I can take a stab at swapping in a separate PR - we could use TensorShape in the ISensor interface, but use int[] in the slightly-higher-level SensorBase interface.
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.
Makes sense. My thoughts (which I didn't fully articulate) were that int[] would still be on the public interface of this, but could use TensorShape internally for things like computing the index. Maybe it's a bit of overkill.
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.
Also, not a requirement for this to get merged.
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.
OK, let me try it in another PR.
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.
Looks good. Just have that one question of why we aren't using TensorShape instead of int[] inside of Sensor.
Support uncompressed visual (i.e. 3d float arrays) observations. This required a small change in how we set up the WriteAdapter on the c# side (to allow passing the shape).
Also added a unit test showing how to go from a 2D float array to an observation.