Add temporal semantics#6
Conversation
lgulich
left a comment
There was a problem hiding this comment.
Looks good overall, just some small comments
| TensorSemantics( | ||
| name="actions", | ||
| ref=actions, | ||
| element_names=[TemporalPeriodMs(100), ["hip", "knee", "ankle"]], |
There was a problem hiding this comment.
So here this would mean that the first axis is time, the second axis is joints?
When I initially read this line (without reading the documentaiton above) I found it a bit unintuitive that TemporalPeriodMs is part of element_names.
But now after reading the comment above it makes sense. Any idea how we could make this more clear at a glance?
There was a problem hiding this comment.
what is your opinion on
TensorSemantics( name="actions", ref=actions, element_names=[TemporalAxis(period_ms=100), ["hip", "knee", "ankle"]],
or is the main issue we have a non-string defined in a field that is called element_names
There was a problem hiding this comment.
yeah I think mainly "element_names" is confusing, since both TemporalPeriodMs / TemporalAxis are both not "names".
Something like the following would be the most natural to me, but this is not back compatible :/
axis_kind=[TemporalAxis(period_ms=100), IndexAxis(names=['hip','knee','ankle'])]
There was a problem hiding this comment.
what if we did this:
TensorSemantics(
name="actions",
ref=actions,
dims=[
TemporalDim(period_ms=100),
NamedDim(names=["hip", "knee", "ankle"]),
],
)
this will be the new recommended api. however we keep the original element_names as a backward compatible wrapper that is the same as dims.
we also assume any list passed in is a NamedDim and normalize it as such. so then the original
element_names= [['a', 'b', 'c'], ['1', '2', '3']]
and
dims = [NamedDim(names=['a', 'b', 'c']), NamedDim(names=['1', '2', '3']]
are equivalent and we can even keep the raw list passing as a short hand.
in the future this also gives us space to add batch dims or make it so some dimensions are variable in size etc. but this will mean the final yaml specs need to change.
element_names field should be something like dim semantics. might organize it like this
dim_semantics: [{type: temporal_dim, period_ms: 100}, {type: batch_dim}, {type: None}, {type: named_dim, names=["hip", "knee", "ankle"]}]
but I am not sure if it is worth it to do this right now or at all. especially because this will for sure require a change on the deployment framework side.
my vote is for now we can just go with the element names idea and in the future if the need for more dimensions arises we can consider updating.
| ref = None, | ||
| kind: InputKindEnum | OutputKindEnum | str | None = None, | ||
| element_names: list | None = None, | ||
| temporal_period_ms: float | int | None = None, |
There was a problem hiding this comment.
do we need the temporal_period_ms here too? if it's set here which axis would this apply to?
I wonder if it's enought to just have the TemporalPeriodMs for element_names?
There was a problem hiding this comment.
you're right. this isn't intended. I think we can get rid of this.
Co-authored-by: lgulich <22480644+lgulich@users.noreply.github.com>
Summary
Adds graph-level and temporal semantic metadata support to LEAPP YAML export.
Changes
GraphConfigsfor graph-level metadata passed throughleapp.compile_graph(...).pipeline.configs, e.g.frequencyand flattenedextravalues.TemporalPeriodMsfor marking a tensor axis as temporal throughTensorSemantics.element_names.__temporal_axis__sentinel andtemporal_period_ms.GraphConfigsandTemporalPeriodMsfrom the publicleapppackage.GraphConfigsandTemporalPeriodMsYAML output.Example
Final yaml: