Merged
Conversation
Closed
tzerrell
reviewed
Nov 2, 2020
tzerrell
left a comment
There was a problem hiding this comment.
Looks good! I have some minor inline comments.
Regarding precision: For now, let's land this with just I32 & FP32 support. There have been precision problems with several ops (see e.g. #41 and @cnamrata15 may have seen some problems with logical ops), and I think these problems might be related. After we investigate those we can return to these specific BinaryConvolution precision problems to see if they still exist.
Thank you for highlighting the precision problems on this op, it will be helpful to know where these are when we go investigating precision problems more generally.
Comment on lines
+41
to
+44
| auto one = cast(edsl::Tensor(1), DType::INT32); | ||
| auto zero = cast(edsl::Tensor(0), DType::INT32); | ||
| input_tensor = edsl::select(I == 0, zero, one); | ||
| filter_tensor = edsl::select(F == 0, zero, one); |
There was a problem hiding this comment.
I am uncertain if this is correct behavior, because OpenVINO currently only implements XNOR_POPCOUNT (and only partially documents what the alternative might be). Your choice looks reasonable, but we don't know what will be expected.
I propose instead we assert IE_ASSERT(mode == ngraph::op::v1::BinaryConvolution::BinaryConvolutionMode::XNOR_POPCOUNT) and only include the true branch of the if. Therefore, if & when OpenVINO adds other BinaryConvolutionModes, we will know to check what the expected implementation is.
Author
There was a problem hiding this comment.
Yes, you are right. I will assert mode as XNOR_POPCOUNT and only include behaviors implemented in OpenVINO.
| #include "ngraph_functions/builders.hpp" | ||
| #include "ngraph_functions/utils/ngraph_helpers.hpp" | ||
|
|
||
| // ! [test_binary_convolution:definition] |
There was a problem hiding this comment.
I don't know what this comment means. Perhaps remove it?
Author
There was a problem hiding this comment.
I just copied and modified them from convolution.hpp, and made them looks consistent.
If it's needed, I will remove them.
There was a problem hiding this comment.
Oh interesting. I looked to see if it appears elsewhere, and found it in docs/IE_PLUGIN_DG/PluginTesting.md. I think it is a tag on the source code to help the documentation find it.
Since we do not want binary_convolution to be found from the documentation for a regular convolution, please remove it. (And do analogously for the other comment like this.)
| protected: | ||
| void SetUp() override; | ||
| }; | ||
| // ! [test_convolution:definition] |
There was a problem hiding this comment.
I don't know what this comment means. Perhaps remove it?
YangleiZouIntel
pushed a commit
that referenced
this pull request
Dec 18, 2020
tzerrell
added a commit
that referenced
this pull request
Dec 18, 2020
* [DOCS] [41549] Fix broken code block in Install OpenVINO from PyPI Repository (openvinotoolkit#2800) * Turned list into headings * fixes * fix * add animation (openvinotoolkit#2865) * Align `time_tests` with master branch from 4021e14 (openvinotoolkit#2881) * Added info on DockerHub CI Framework (openvinotoolkit#2919) * Feature/azaytsev/cherry pick pr2541 to 2021 1 (openvinotoolkit#2960) * added OpenVINO Model Server to docs (openvinotoolkit#2541) * added OpenVINO Model Server * updated documentation to include valid links * minor fixes * Fixed links and style * Update README.md fixed links to model_server * more corrections * dropped reference in ie_docs and minor fixes * Update README.md Fixed links to Inference Engine pages Co-authored-by: Alina Alborova <alina.alborova@intel.com> Co-authored-by: Andrey Zaytsev <andrey.zaytsev@intel.com> * Added Model Server docs to 2021/1 Co-authored-by: Trawinski, Dariusz <dariusz.trawinski@intel.com> Co-authored-by: Alina Alborova <alina.alborova@intel.com> * See Also sections in MO Guide (openvinotoolkit#2770) * convert to doxygen comments * layouts and code comments * separate layout * Changed layouts * Removed FPGA from the documentation * Updated according to CVS-38225 * some changes * Made changes to benchmarks according to review comments * Added logo info to the Legal_Information, updated Ubuntu, CentOS supported versions * Updated supported Intel® Core™ processors list * Fixed table formatting * update api layouts * Added new index page with overview * Changed CMake and Python versions * Fixed links * some layout changes * some layout changes * some layout changes * COnverted svg images to png * layouts * update layout * Added a label for nGraph_Python_API.md * fixed links * Fixed image * removed links to ../IE_DG/Introduction.md * Removed links to tools overview page as removed * some changes * Remove link to Integrate_your_kernels_into_IE.md * remove openvino_docs_IE_DG_Graph_debug_capabilities from layout as it was removed * update layouts * Post-release fixes and installation path changes * Added PIP installation and Build from Source to the layout * Fixed formatting issue, removed broken link * Renamed section EXAMPLES to RESOURCES according to review comments * add mo faq navigation by url param * Removed DLDT description * Pt 1 * Update Deep_Learning_Model_Optimizer_DevGuide.md * Extra file * Update IR_and_opsets.md * Update Known_Issues_Limitations.md * Update Config_Model_Optimizer.md * Update Convert_Model_From_Kaldi.md * Update Convert_Model_From_Kaldi.md * Update Convert_Model_From_MxNet.md * Update Convert_Model_From_ONNX.md * Update Convert_Model_From_TensorFlow.md * Update Converting_Model_General.md * Update Cutting_Model.md * Update IR_suitable_for_INT8_inference.md * Update Aspire_Tdnn_Model.md * Update Convert_Model_From_Caffe.md * Update Convert_Model_From_TensorFlow.md * Update Convert_Model_From_MxNet.md * Update Convert_Model_From_Kaldi.md * Added references to other fws from each fw * Fixed broken links * Fixed broken links * fixes * fixes * Fixed wrong links Co-authored-by: Nikolay Tyukaev <ntyukaev_lo@jenkins.inn.intel.com> Co-authored-by: Andrey Zaytsev <andrey.zaytsev@intel.com> Co-authored-by: Tyukaev <nikolay.tyukaev@intel.com> * Fixes (openvinotoolkit#3105) * Renamed Benchmark App into Benchmark Tool in the menu (openvinotoolkit#3032) * [DOC] Update Docker install guide (openvinotoolkit#3055) (openvinotoolkit#3200) * [DOC] Update Docker install guide * [DOC] Add proxy for Windows Docker install guide * [DOC] move up prebuilt images section * Update installing-openvino-linux.md * Update installing-openvino-docker-linux.md * Update installing-openvino-docker-linux.md Formatting fixes * Update installing-openvino-docker-linux.md Fixed formatting issues * Update installing-openvino-docker-windows.md Minor fixes * Update installing-openvino-docker-linux.md Fixed formatting issues * [DOC] update text with CPU image, remove proxy for win * Update installing-openvino-docker-windows.md Minor fixes * Update installing-openvino-docker-windows.md Minor fix * Update installing-openvino-docker-windows.md Minor fix * Update installing-openvino-docker-windows.md Co-authored-by: Andrey Zaytsev <andrey.zaytsev@intel.com> (cherry picked from commit 4a09888) * Align time_tests with master (openvinotoolkit#3238) * Align time_tests with master * Fix "results" uploading to DB in time_tests * Add new model to `tgl_test_config.yml` * Fix onnx tests versions (openvinotoolkit#3240) * [40929] DL Workbench in Get Started (openvinotoolkit#2740) * Initial commit * Added the doc * More instructions and images * Added slide * Borders for screenshots * fixes * Fixes * Added link to Benchmark app * Replaced the image * tiny fix * tiny fix * Links to DL Workbench Installation Guide (openvinotoolkit#2861) * Links to WB * Changed wording * Changed wording * Fixes * Changes the wording * Minor corrections * Removed an extra point * [41545] Add links to DL Workbench from components that are available in the DL WB (openvinotoolkit#2801) * Added links to MO and Benchmark App * Changed wording * Fixes a link * fixed a link * Changed the wording * Feature/azaytsev/change layout (openvinotoolkit#3295) * Changes according to feedback comments * Replaced @ref's with html links * Fixed links, added a title page for installing from repos and images, fixed formatting issues * Added links * minor fix * Added DL Streamer to the list of components installed by default * Link fixes * Link fixes * ovms doc fix (openvinotoolkit#2988) * added OpenVINO Model Server * ovms doc fixes Co-authored-by: Trawinski, Dariusz <dariusz.trawinski@intel.com> * Add several new models to `tgl_test_config.yml` in time_tests (openvinotoolkit#3269) * Fix wrong path for `yolo-v2-tiny-ava-0001` for time_tests * Add several new models to `tgl_test_config.yml` in time_tests * Fix a typo in DL Workbench Get Started (openvinotoolkit#3338) * Fixed a typo * Update openvino_docs.xml Co-authored-by: Andrey Zaytsev <andrey.zaytsev@intel.com> * ops math formula fix (openvinotoolkit#3333) Co-authored-by: Nikolay Tyukaev <ntyukaev_lo@jenkins.inn.intel.com> * Fix paths for `squeezenet1.1` in time_tests config (openvinotoolkit#3416) * GNA Plugin doc review (openvinotoolkit#2922) * Doc review * Addressed comments * Removed an inexistent link * Port PlaidML plugin forward to 2021.1 (#32) Adds PlaidML plugin to this repo on the 2021.1 branch * Enable testing of BatchNorm (#33) * Require specific path to shared library (#34) * enable separate source/so paths * fix tabs * Fix multiple outputs and add Split (#42) Fixes the problem with multiple outputs in the PlaidML plugin described in #36. Adds the Split operation with tests, which utilizes multiple outputs. * Swish (#47) * Add Reverse & tests to PlaidML Plugin (#35) * Make separate PlaidMLProgramBuilder (#92) * Variadic Split (#91) * variadic split w/o -1 * -1 implemented * Fix -1 support in variadic split and test it * Style consistency * Cleanup * Fix exception message * Add BinaryConvolution (#93) * Add working tests back (#97) * Attempt to add tests back * Change op name for logical_and to match tests * Add Tests for *Convert *Convolution_Backprop_Data *Fake_quantize *SoftMax *Tile *Transpose * Fix comparison and logical tests, use IE ref mode for now * Remove cumsum and logical tests * Remove comparison tests and it's fixes * Add bucketize op and tests (#90) * Add extract image patches op (#96) * Hswish via ReLU (#95) * Hswish via ReLU * ReLU max_value used * Add reorg_yolo op (#101) * Remove conv bprop & fake quant tests (#106) * add EmbeddingBagOffsetsSum op and tests (#100) * Add LSTMCell (#102) * Add RNNCell (#109) Co-authored-by: Ling, Liyang <liyang.ling@intel.com> * Add space_to_batch op (#104) * Add tests for MinMax, DepthToSpace (#105) * Add GELU (#107) * Add GRUCell (#110) Co-authored-by: Ling, Liyang <liyang.ling@intel.com> * Fix support for using OpenVINO as a subproject (#111) * Build fixes for newer compilers (#113) * add EmbeddingBagPackedSum op and tests (#114) Co-authored-by: Tim Zerrell <tim.zerrell@intel.com> * Add shuffle_channels op and test. (#112) * Tests for squared difference op (#115) Co-authored-by: Tim Zerrell <tim.zerrell@intel.com> * Add acosh, asinh, atanh into tests (#118) * Reverse sequence (#116) * Add PriorBox op and test. (#117) * Remove obsolete PlaidML code (#120) Co-authored-by: Alina Alborova <alina.alborova@intel.com> Co-authored-by: Nikolay Tyukaev <nikolay.tyukaev@intel.com> Co-authored-by: Vitaliy Urusovskij <vitaliy.urusovskij@intel.com> Co-authored-by: Andrey Zaytsev <andrey.zaytsev@intel.com> Co-authored-by: Trawinski, Dariusz <dariusz.trawinski@intel.com> Co-authored-by: Nikolay Tyukaev <ntyukaev_lo@jenkins.inn.intel.com> Co-authored-by: Kate Generalova <kate.generalova@intel.com> Co-authored-by: Rafal Blaczkowski <rafal.blaczkowski@intel.com> Co-authored-by: Tim Zerrell <tim.zerrell@intel.com> Co-authored-by: Brian Retford <brian.retford@intel.com> Co-authored-by: Michael Yi <michael.w.yi@intel.com> Co-authored-by: Liyang Ling <liyang.ling@intel.com> Co-authored-by: Namrata Choudhury <namrata.choudhury@intel.com> Co-authored-by: Xin Wang <xin1.wang@intel.com> Co-authored-by: xinghong chen <xinghong.chen@intel.com> Co-authored-by: Zhibin Li <haoyouab@gmail.com> Co-authored-by: Frank Laub <frank.laub@intel.com>
YangleiZouIntel
pushed a commit
that referenced
this pull request
Jan 14, 2021
YangleiZouIntel
pushed a commit
that referenced
this pull request
Feb 2, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
According to description in OpenVINO BinaryConvolution, strategy here is converting both input tensors to tensors filled with binary values(0/1, -1/1), and pass them to
plaidml::op::convolution.After testing, only
I32andFP32can pass all test cases. It seems OpenVINO doc does not explicitly mention the datatype.So, if that's okay for this op, or should we make it works for binary datatype?