Skip to content

Revise Broadcast reference implementation#2715

Merged
GlebKazantaev merged 42 commits intoopenvinotoolkit:masterfrom
mbencer:mbencer/ReviseBroadcast
Nov 10, 2020
Merged

Revise Broadcast reference implementation#2715
GlebKazantaev merged 42 commits intoopenvinotoolkit:masterfrom
mbencer:mbencer/ReviseBroadcast

Conversation

@mbencer
Copy link
Copy Markdown
Contributor

@mbencer mbencer commented Oct 19, 2020

This PR depends on #2641 (tile reference implementation) which should be merged first.

  • improved Broadcast ref implementation
  • reduced binary size by not using templates
  • added single layer tests
  • fixed handling boolean type for CPU plugin

@mbencer mbencer requested review from e-nugmanova and pszmel October 19, 2020 10:29
@mbencer mbencer self-assigned this Oct 19, 2020
@mbencer mbencer requested a review from ilyachur October 19, 2020 10:54
Copy link
Copy Markdown
Contributor

@iefode iefode left a comment

Choose a reason for hiding this comment

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

Test part LGTM

@mbencer mbencer requested a review from ilyachur October 27, 2020 14:25
Copy link
Copy Markdown
Contributor

@ilyachur ilyachur left a comment

Choose a reason for hiding this comment

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

LGTM


// Copy and repeat data for innermost axis as many times as described in the repeats parameter
while (axis <= indices.size())
try
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you explain why you added this try/catch and changed the Tile code?

Should we add more tests for Tile op?

Copy link
Copy Markdown
Contributor Author

@mbencer mbencer Oct 29, 2020

Choose a reason for hiding this comment

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

@ilyachur I've observed exception in e2e after merge 0c5e17d to my PR with very limited error message. It is hard to reproduce it locally. This try/catch is only to collect info about error scenario and I'll revert it.
To the number of tests, probably right, we should add more

@ilyachur ilyachur self-requested a review October 29, 2020 03:48
@GlebKazantaev GlebKazantaev merged commit eeafc8e into openvinotoolkit:master Nov 10, 2020
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 16, 2020
* change tile reference implementation

* remove tile tests from interpreter manifest

* add repeats parameter to tile

* improve tile reference implementation

* add repeats parameter to tile reference call in tile evaluate method

* style apply

* include <numeric>

* add unnamed namespace to helper functions. Change stdio.h to cstdio. Change input_rank to be constant int

* add const reference to parameter repeats in tile reference function

* change createPitches function to use partial_sum instead of accumulate

* change a little bit createPitches function

* style-apply

* fix function naming

* style-apply

* fix calling functions name bug

* Add description of create_pitches function

* first version with debug logs

* reduce footprint

* single layer tests

* added more tests

* fixed handling bool type

* styles applied

* fix tile

* [ONLY DEBUG] print error scenario message

* fixed problem with e2e tests

* fixed casting of start_axis for numpy mode

Co-authored-by: pszmel <piotr.szmelczynski@intel.com>
@mbencer mbencer deleted the mbencer/ReviseBroadcast branch March 4, 2021 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants