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

[MXNET-879] ONNX export: Logical operators#12852

Merged
nswamy merged 1 commit intoapache:masterfrom
vandanavk:onnx_operators
Dec 3, 2018
Merged

[MXNET-879] ONNX export: Logical operators#12852
nswamy merged 1 commit intoapache:masterfrom
vandanavk:onnx_operators

Conversation

@vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Oct 17, 2018 •
edited
Loading

Description

Added support for exporting And, Or, Xor, Not to ONNX.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Export the 4 operators in _op_translations.py
  • Added tests for the 4 operators
  • Removed test_logical_ from import because the tests added include import, and ONNX node tests don't have a test for test_logical.

Comments

@vandanavk vandanavk requested a review from szha as a code owner October 17, 2018 18:52
anirudhacharya
anirudhacharya reviewed Oct 17, 2018
View reviewed changes
@Roshrini
Copy link
Member

Roshrini commented Oct 17, 2018

@mxnet-label-bot [ONNX, pr-awaiting-review]

Roshrini
Roshrini reviewed Oct 17, 2018
View reviewed changes
tests/python-pytest/onnx/export/mxnet_export_test.py Outdated

@with_seed()
def test_logical_and():
"""Test for logical and in onnx operators."""
Copy link
Member

@Roshrini Roshrini Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have common method get_input() for getting input arrays which we can use in all functions?

Copy link
Contributor Author

@vandanavk vandanavk Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Roshrini as discussed offline, added a helper function to get integer inputs

@marcoabreu marcoabreu added ONNX pr-awaiting-review PR is waiting for code review labels Oct 17, 2018
@vandanavk vandanavk force-pushed the onnx_operators branch 2 times, most recently from c9e2672 to 2bc1dae Compare October 22, 2018 23:07
@ankkhedia
Copy link
Contributor

ankkhedia commented Oct 30, 2018

@Roshrini @anirudhacharya It seems that the review comments has been addressed. Could you please take a look?

@anirudhacharya
Copy link
Member

anirudhacharya commented Oct 30, 2018

I think @vandanavk has another PR to clean up the export module in ONNX, that PR should get merged and after rebasing this on top of that we can revisit this PR.

@vandanavk
Copy link
Contributor Author

vandanavk commented Nov 1, 2018

Rebased. Ready for review again

@vandanavk
Copy link
Contributor Author

vandanavk commented Nov 1, 2018

@zhreshold for review

@Roshrini
Copy link
Member

Roshrini commented Nov 2, 2018

LGTM

@ankkhedia
Copy link
Contributor

ankkhedia commented Nov 2, 2018

@zhreshold @sandeep-krishnamurthy @nswamy Could you please help with review/merge?

nswamy
nswamy reviewed Nov 4, 2018
View reviewed changes
tests/python-pytest/onnx/export/mxnet_export_test.py Outdated
npt.assert_almost_equal(result, numpy_op)

@with_seed()
def test_logical_and():
Copy link
Member

@nswamy nswamy Nov 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for these simple operators, instead of repeating test code for each of them, can you create a map[operator] = (input, output) and run all operators tests in one test method?

@kalyc
Copy link
Contributor

kalyc commented Nov 13, 2018

Ping @vandanavk any update on the PR?

@vandanavk
Copy link
Contributor Author

vandanavk commented Nov 13, 2018

@kalyc I'm currently working on some GitHub issues. Will get back to this in a couple of weeks (after cleaning up ONNX test code)

@vandanavk vandanavk force-pushed the onnx_operators branch 6 times, most recently from a3f8e51 to 74a5a94 Compare November 26, 2018 17:45
@vandanavk
Copy link
Contributor Author

vandanavk commented Nov 26, 2018

@mxnet-label-bot update [ONNX, pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Nov 26, 2018
@stu1130
Copy link
Contributor

stu1130 commented Nov 26, 2018

@nswamy could you merge this PR?

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Nov 29, 2018
View reviewed changes
Copy link

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.
Can you please rebase?

@vandanavk vandanavk force-pushed the onnx_operators branch 2 times, most recently from c0569f9 to ad18562 Compare November 29, 2018 17:23
@nswamy nswamy merged commit 7d44deb into apache:master Dec 3, 2018
sergeykolychev pushed a commit that referenced this pull request Dec 5, 2018
@srochel @sergeykolychev
Bumped minor version from 1.4.0 to 1.5.0 on master, updated License f…
40db619 
…ile (#13478)

* updated to v1.5.0

* Bumped minor version from 1.4.0 to 1.5.0 on master

* added Anirudh as maintainer for R package

... adding something useful and re-trigger PR check

* Updated license file for clojure, onnx-tensorrt, gtest, R-package

* Get the correct include path in pip package (#13452)

* add find_include_path API

* address reviewer comment

* change return type from list to string

* add unit test

* address reviewer comment

* address reviewer comment

* address reviewer comment

* address reviewer comment

* fix include path problem in pip package

* add comment

* fix lint error

* address reviewer comment

* address reviewer comment

* Use ~/.ccache as default ccache directory so is not cache is not erased on reboot (#13431)

* Skip flaky test #13446 (#13480)

* Rewrite dataloader with process pool, improves responsiveness and reliability (#13447)

* fix recordio.py

* rewrite dataloader with pool

* fix batch as tuple

* fix prefetching

* fix pylint

* picklable function

* use pickle

* add missing commit

* Fix errors in docstrings for subgraph op; use code directive (#13463)

* [MXNET-1158] JVM Memory Management Documentation (#13105)

* update train_mnist

* Add documentation for JVM Memory Management

* update doc

* address nit picks

* address nit picks

* Grammar and clarity edits for memory management doc

* Edits for scala memory management

* Update memory-management.md

* Update memory-management.md

* Update memory-management.md

* capitalization fix

* Update row_sparse tutorial (#13414)

Update row_sparse tutorial

* Add resiliency to onnx export code (#13426)

* Added resiliency to onnx export code

- With previous infer-shape implementation, if input shape was list instead of tuple or if extra non-existent parameters were provided, the code would still work. The fixes in this commit make sure that behavior is restored to prevent any compatibility issues with existing export code.

* Fixed name of net in unittest

* Fix pylint

* [MXNET-1185] Support large array in several operators (part 1) (#13418)

* fix a few operators with large arrays (# of elements)

* fix bug in broadcast_div and add tests

* address reviewer comment

* add unit test

* add empty line

* retrigger CI

* [MXNET-1210 ] Gluon Audio - Example (#13325)

* Initialized the example

* Addressed PR comments, about existing synset.txt file - no overwrite

* RST - docstring issues fixed

* added README

* Addressed PR comments

* Addressed PR comments, checking Divide by 0

* Raising error if format is not supported.

* changed a line for ndarray of labels

* Trigger CI

* Trigger CI

* PR comments addressed around skip_header argument

* Addressed PR comments around librosa import

* PR Comments

* Passing lazy=lazy from argument

* Added PR comments, labels to README.MD

* Trigger CI

* Addressing PR Comments in README

* Modified README.md

* Added example under audio folder

* Retrigger CI

* Retrigger CI

* ONNX export: Instance normalization, Shape (#12920)

* ONNX import/export: Make backend_rep common

* ONNX export: Instance Normalization

* ONNX export: Shape operator

* Clarify dependency on OpenCV in CNN Visualization tutorial. (#13495)

* clarify ops faq regarding docs strings (#13492)

* Add graph_compact operator. (#13436)

* add graph_compact.

* fix.

* add doc.

* add tests for graph_compact.

* address comments.

* update docs.

* trigger CI

* Deprecate Jenkinsfile (#13474)

* update github location for sampled_block.py (#13508)

Updated to https://github.com/dmlc/gluon-nlp/blob/master/src/gluonnlp/model/sampled_block.py

* #13453 [Clojure] - Add Spec Validations to the Optimizer namespace (#13499)

* ONNX export: Logical operators (#12852)

* Fix cmake options parsing in dev_menu (#13458)

Add GPU+MKLDNN unittests to dev_menu

* Revert "Manually track num_max_thread (#12380)" (#13501)

This reverts commit 7541021.

* Feature/mkldnn static 2 (#13503)

* build mkldnn as static lib

* update makefile to statically build mkldnn

* build static mkldnn

* fix static name

* fix static name

* update static for mac

* rename mkldnn dep in ci

* remove moving mkldnn dynamic lib

* remove commented code

* remove mkldnn dnaymic for unitest

* force static for mkldnn lib

* remove dynamic mkldnn bind

* only link windows

* add mkldnn.mk

* try force linking

* remove mkldnn dynanmic check

* remove test mkldnn install

* fix spacing

* fix index

* add artifacts

* add comment about windows

* remove static

* update makefile

* fix toctree Sphinx errors (#13489)

* fix toctree errors

* nudging file for CI

* Disabled flaky test test_gluon_data.test_recordimage_dataset_with_data_loader_multiworker (#13527)

* [MXNET-1234] Fix shape inference problems in Activation backward (#13409)

* Provide a failing test for ReLU activation shape inference bug

* Fix Activation backward shape inference

fixes: #13333

* Add softsign Activation to test_gluon.py

* Use activation in GPU if we are using CUDNN and not MKLDNN as it's happening right now

* Don't disable MKLDNN
@vandanavk vandanavk mentioned this pull request Dec 12, 2018
Merged
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@szha szha Awaiting requested review from szha szha is a code owner

5 more reviewers

@nswamy nswamy nswamy left review comments

@anirudhacharya anirudhacharya anirudhacharya left review comments

@Roshrini Roshrini Roshrini left review comments

@zhreshold zhreshold zhreshold approved these changes

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ONNX pr-awaiting-merge Review and CI is complete. Ready to Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@vandanavk @Roshrini @ankkhedia @anirudhacharya @kalyc @stu1130 @nswamy @zhreshold @sandeep-krishnamurthy @marcoabreu