[tmva][sofie] register Swish operator in the ONNX parser#22564
Conversation
|
kindly review, thanks. On a side note, I had a doubt whether we would need a clean build for this as well? Since for the last 2 PR there was some caching issue where simple build was failing. @guitargeek |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
Thank you for the PR. Interesting we add the suppot in the ROPerator but missed the parser.
Nice you included also the test.
We can maybe simplify the code, removing the template parameter in ROperator_Swish
|
|
||
| std::string output_name = nodeproto.output(0); | ||
|
|
||
| switch (input_type) { |
There was a problem hiding this comment.
Do we need here to have different instantiation depending on input type?
I would drop the template type in ROperator_Swish, it is not needed
There was a problem hiding this comment.
Thanks for the review. I had kept the template following the existing activation function parsers(Sigmoid, etc.) for consistency. But you're correct, the template type isnt really used. I'll update the PR and simplfy the parser accordingly
Test Results 21 files 21 suites 3d 7h 46m 2s ⏱️ For more details on these failures, see this check. Results for commit 9b903d1. |
|
i've updated the PR with the requested changes, kindly review, thanks |
3 participants
Changes or fixes:
This PR registers the existing Swish operator in the ONNX parser.
ROperator_Swishwas implemented but never registered, so a model with the standard ONNXSwishop (opset 24) failed with "Operator type Swish is not yet supported".I've added
ParseSwishand it also reads the optionalalphaattribute and rejectes when alpha!=1, since right now the op only implements alpha=1. Correspondingly "Swish" has been registered inRModelParser_ONNX.Also added a
Swishtest model with reference and a test patch/Checklist:
This PR fixes #22552