-
Notifications
You must be signed in to change notification settings - Fork 615
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
Add per-frame operator #3723
Add per-frame operator #3723
Conversation
!build |
dali/pipeline/operator/op_schema.h
Outdated
DLL_PUBLIC inline OpSchema& SupportPerFrameArgs() { | ||
support_per_frame_ = true; | ||
return *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 what we might need fine grain control of which exact argument makes sense to be provided per frame.
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 should be inferred from argument descriptions.
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.
Interesting idea; but is such info needed at all if we decided to mark arguments and mention per-frame support in their descriptions?
@@ -130,6 +130,8 @@ def _get_kwargs(schema): | |||
default_value = ast.literal_eval(default_value_string) | |||
type_name += ", default = `{}`".format(_default_converter(dtype, default_value)) | |||
doc += schema.GetArgumentDox(arg) | |||
if schema.ArgSupportsPerFrameInput(arg): |
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 requires the other PR to work, doesn't it?
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.
Yeah, it needs to be rebased.
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 wanted to have it ready here so that it could be read, reviewed and discussed if needed.
Unless something significantly changes, I can review this instead of @prak-nv, but for now it is already approved. Add me if you need me. |
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
… sequneces need to have matching shapes Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
!build |
CI MESSAGE: [4455657]: BUILD STARTED |
CI MESSAGE: [4455657]: BUILD PASSED |
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
!build |
CI MESSAGE: [4462185]: BUILD STARTED |
CI MESSAGE: [4462185]: BUILD FAILED |
CI MESSAGE: [4462185]: BUILD PASSED |
* Add per-frame operator * Use per-frame operator in sequence operator tests * Link to per-frame operator docs from arguments supporting per-frame input Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
* Add per-frame operator * Use per-frame operator in sequence operator tests * Link to per-frame operator docs from arguments supporting per-frame input Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Category:
New feature (non-breaking change which adds functionality)
Description:
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
The operator is CPU only for the same reason the fn.transofrms are - it is supposed to be used only on arguments to other operators.
Checklist
Tests
Documentation
DALI team only
Requirements
REQ IDs: PERFR.01-PERFR.05
JIRA TASK: DALI-2566