[MLA-1634] Compression spec#5164
Conversation
| }; | ||
| } | ||
|
|
||
| internal static bool IsTrivialMapping(ISensor sensor) |
There was a problem hiding this comment.
Moved this logic to CompressionSpec
| } | ||
|
|
||
| [Test] | ||
| public void TestIsTrivialMapping() |
There was a problem hiding this comment.
Moved to com.unity.ml-agents/Tests/Editor/Sensor/CompressionSpecTests.cs
surfnerd
left a comment
surfnerd
left a comment
There was a problem hiding this comment.
feels so much cleaner. good stuff! 
|
@dongruoping I forgot to tag you before - does this look OK with the channel mapping? |
dongruoping
left a comment
dongruoping
left a comment
There was a problem hiding this comment.
Looks good to me.
Left a few comments but are more like thoughts on what we can do later.
| public virtual CompressionSpec GetCompressionSpec() | ||
| { | ||
| return SensorCompressionType.None; | ||
| return CompressionSpec.Default(); | ||
| } |
There was a problem hiding this comment.
A potential improvement of this is to have a m_CompressionSpec and return that like we do with m_ObservationSpec.
Not sure if it's worthy though given this looks pretty light weight.
There was a problem hiding this comment.
We cached the shapes previously, because they required allocating memory. I kept that pattern for the ObservationSpecs - it's not really necessary for performance (struct with InplaceArray, so no allocations), but it's not a bad idea since they shouldn't change at runtime. I don't think it's necessary for CompressionSpecs.
| { | ||
| m_SensorCompressionType = sensorCompressionType, | ||
| m_CompressedChannelMapping = compressedChannelMapping | ||
| }; |
There was a problem hiding this comment.
technically you can pass in SensorCompressionType.None and non-null mapping?
There was a problem hiding this comment.
Yeah, you can. I think I'll just make this the constructor instead of using a static method.
| /// Constrct stacked CompressedChannelMapping. | ||
| /// Construct stacked CompressedChannelMapping. | ||
| /// </summary> | ||
| internal int[] ConstructStackedCompressedChannelMapping(ISensor wrappedSenesor) |
There was a problem hiding this comment.
Thought a bit about moving this to the CompressionSpec constructor, don't think it's necessary here though.
But if one day we add more compression types it might make sense to move this so we can do something like CompressionSpec(PNG, num_layer=4, stacking=2) and have it automatically setup for different compression.
There was a problem hiding this comment.
Yeah, I think we can move more utility methods into CompressionSpec (even if they stay internal static). I think I'll leave this here for now, but we can move it later if it makes sense (and there's another use case)
3 participants
Proposed change(s)
Change ISensor interface, and remove ISparseChannelSensor interface.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments