Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise Concat op #4543

Merged
merged 5 commits into from
Mar 15, 2021
Merged

Revise Concat op #4543

merged 5 commits into from
Mar 15, 2021

Conversation

blesniewski
Copy link
Contributor

@blesniewski blesniewski commented Mar 1, 2021

Details:

  • Revise operator class:
    validate_and_infer_types needed additional check for an edge-case axis attribute value.
    Unit tests were added to cover this change.
  • Revise implementation performance-wise - this was done by Mateusz B. here and Patryk E. here.
  • Add SLT - already existing.

Tickets:

  • 37427

@blesniewski blesniewski added the category: Core OpenVINO Core (aka ngraph) label Mar 1, 2021
@blesniewski blesniewski self-assigned this Mar 1, 2021
@blesniewski blesniewski marked this pull request as ready for review March 2, 2021 12:52
@blesniewski blesniewski requested a review from a team March 2, 2021 12:52
@ilyachur
Copy link
Contributor

ilyachur commented Mar 3, 2021

@blesniewski Please set the milestone

@jdanieck jdanieck added this to the 2021.4 milestone Mar 3, 2021
@lazarevevgeny
Copy link
Contributor

Why "DO NOT MERGE"? If this is a regression in 2021.3 then we should merge it in 2021.3.

@jdanieck
Copy link
Contributor

jdanieck commented Mar 3, 2021

Why "DO NOT MERGE"? If this is a regression in 2021.3 then we should merge it in 2021.3.

I asked not to merge anything which is not critical because today is the Code Freeze and I don't want to take a risk to potentially introduce new issues which we didn't think of right now. I don't see this change as regression or critical, it just adds additional input validation and as I experienced before such additional check can break models from E2E (but in theory it shouldn't).

@blesniewski
Copy link
Contributor Author

@openvinotoolkit/openvino-ngraph-maintainers This PR can be merged.

@ilyachur ilyachur merged commit 635ffc7 into openvinotoolkit:master Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants