Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[COREML] Update the json getter #8698

Merged
merged 2 commits into from Nov 19, 2017
Merged

[COREML] Update the json getter #8698

merged 2 commits into from Nov 19, 2017

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Nov 17, 2017

Description

(Brief description on what this PR is about)

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • For user-facing API changes, API doc string has been updated. For new C++ functions in header files, their functionalities and arguments are well-documented.
  • To my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

@tqchen
Copy link
Member Author

tqchen commented Nov 17, 2017

cc @piiswrong @pracheer

@piiswrong
Copy link
Contributor

I think attr was also referenced in the other files?

@piiswrong
Copy link
Contributor

Also this needs to support both attr and attrs because we need to handle json saved by previous versions

@tqchen
Copy link
Member Author

tqchen commented Nov 17, 2017

Already did that( support attr and attrs) they are only refered in layer file

@pracheer
Copy link
Contributor

Looks good to me.

One nitpick suggestion if you think it is useful: can you add some documentation to _get_attrs() with some info on attr/attrs story (like models created w/ which mxnet version will have "attrs" as part of their defn etc)?

@tqchen
Copy link
Member Author

tqchen commented Nov 17, 2017

@pracheer updated,

@tqchen
Copy link
Member Author

tqchen commented Nov 19, 2017

Merge this ?

@pracheer
Copy link
Contributor

Yes please!

@piiswrong piiswrong merged commit 38a032c into apache:master Nov 19, 2017
eric-haibin-lin pushed a commit to eric-haibin-lin/mxnet that referenced this pull request Dec 3, 2017
* [COREML] Update the json getter

* add docstring
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* [COREML] Update the json getter

* add docstring
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants