-
Notifications
You must be signed in to change notification settings - Fork 3.7k
First Draft EyeLike CPU OP9 #121
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
Conversation
| 9, | ||
| KernelDefBuilder().TypeConstraint("T1", | ||
| std::vector<MLDataType>{ | ||
| DataTypeImpl::GetTensorType<float>(), |
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 the entire list of supported types as per ONNX spec... should I add all of them?
tensor(float16), tensor(float), tensor(double), tensor(int8), tensor(int16), tensor(int32), tensor(int64), tensor(uint8), tensor(uint16), tensor(uint32), tensor(uint64), tensor(bool)
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.
we could add the types step by step based on requirement. Is there any models require other types?
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 don't know if any model actually needs this operator right now. I am adding this as part of OP9 support. Do we maintain this info somewhere? How do I check?
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.
By default, we start with tensor(float) and add other types if need be.
pranavsharma
left a comment
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.
Did you look into Eigen? They support Nd matrices and operations on them.
| public: | ||
| EyeLike(const OpKernelInfo& info) : OpKernel(info) { | ||
| if (!info.GetAttr("k", &k_).IsOK()) { | ||
| ONNXRUNTIME_THROW("Missing 'k' attribute value"); |
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 like 'k' is optional based on the spec in which case we don't have to throw. #Resolved
| 9, | ||
| KernelDefBuilder().TypeConstraint("T1", | ||
| std::vector<MLDataType>{ | ||
| DataTypeImpl::GetTensorType<float>(), |
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.
we could add the types step by step based on requirement. Is there any models require other types?
| input_dims[1]); | ||
| output_mat.setZero(); | ||
|
|
||
| if ((k_ >= 0 && k_ >= input_dims[1]) || (k_ < 0 && std::abs(k_) >= input_dims[0])) { |
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.
is this an error condition?
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.
Not really an error. If K is greater than row(for lower diag) or col (in case of upper diag) then we terminate early and return a tensor with all zeroes.
* Add null check for node arguments. * Add null check to Shape property of node arg.
Adding implementation for new op-9 operator eyelike