ngraph-mxnet icon indicating copy to clipboard operation
ngraph-mxnet copied to clipboard

Have bridge code use `origin_node_->attrs.parsed` instead of `origin_node_->attrs.dict`

Open cconvey opened this issue 7 years ago • 1 comments

E.g., in src/ngraph/ngraph_graph.cc:

#include "../../src/operator/nn/deconvolution-inl.h"
...
const auto & op_params = dmlc::get<mxnet::op::DeconvolutionParam>(orig_node_->attrs.parsed);
... op_params.stride ...

We can use this approach for all new bridge code, and optionally revise existing bridge code to use this approach.

Pros:

  • We avoid re-parsing strings to more explicit C++ types.
  • We avoid duplicating other MXnet code that sets defaults, which is a potential source of bugs.
  • Possibly more idiomatic, so it might reduce resistance to upstreaming.

Cons:

  • We'll need one extra #include to access each op's ...Params struct.

cconvey avatar May 24 '18 18:05 cconvey

I think we need to do this, but we also need to refactor the ops into multiple files to isolate the includes at the same time.

mbrookhart avatar May 29 '18 16:05 mbrookhart