-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-344] [ONNX-MXNet] Add new Operator Translations for ONNX import module #11140
Conversation
gluonImport # Conflicts: # python/mxnet/contrib/onnx/__init__.py # python/mxnet/contrib/onnx/_import/import_model.py # python/mxnet/contrib/onnx/_import/import_onnx.py
newOps # Conflicts: # python/mxnet/contrib/onnx/_import/import_to_gluon.py # python/mxnet/contrib/onnx/_import/op_translations.py # tests/python-pytest/onnx/import/gluon_backend.py # tests/python-pytest/onnx/import/gluon_backend_test.py # tests/python-pytest/onnx/import/mxnet_backend.py # tests/python-pytest/onnx/import/mxnet_backend_test.py # tests/python-pytest/onnx/import/test_cases.py
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.
Thanks @anirudhacharya for your contributions!
Overall looks good to me.
2 questions below and we are ready to go.
if 'a_max' not in new_attrs: | ||
new_attrs = translation_utils._add_extra_attributes(new_attrs, {'a_max' : np.inf}) | ||
if 'a_min' not in new_attrs: | ||
new_attrs = translation_utils._add_extra_attributes(new_attrs, {'a_min' : -np.inf}) |
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.
can we use np.ninf? I think that is float, please check once.
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.
np.inf can be used as float - https://stackoverflow.com/questions/42315541/difference-between-np-inf-and-floatinf
def reduce_log_sum(attrs, inputs, proto_obj): | ||
"""Reduce the array along a given axis by log sum value""" | ||
keep_dims = True if 'keepdims' not in attrs else attrs.get('keepdims') | ||
sum_op = symbol.sum(inputs[0], axis=attrs.get('axes'), |
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.
Why inputs[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.
ReduceLogSum op takes has input tensor - https://github.com/onnx/onnx/blob/master/docs/Operators.md#ReduceLogSum. And while translating this ONNX operator into MXNet, we are splitting the reduceLogSum op to sum
over a given axis and then log
operator.
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.
it has been similarly used in other operator translations, for example here - https://github.com/apache/incubator-mxnet/pull/11140/files/6f518f13413387b40a5c45d96bd2c1d2decf7804#diff-ea46c490a749b158ef8986126da6bc12R284
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.
LGTM. Thanks.
…t module (apache#11140) * gluon import * gluon tests * shape issues. * remove the dim_change list * onnx backend tests * changes to match onnx op set version 7 * fix * lint fix * add new folder * fix * fix * rename test file * tile topk clip * more ops * fix * fix
…t module (apache#11140) * gluon import * gluon tests * shape issues. * remove the dim_change list * onnx backend tests * changes to match onnx op set version 7 * fix * lint fix * add new folder * fix * fix * rename test file * tile topk clip * more ops * fix * fix
Description
Add translations for the following operators.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments