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

Implement OpenVINO Decomposition for numpy.hstack Operation #21017

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

Hmm-1224
Copy link

@Hmm-1224 Hmm-1224 commented Mar 12, 2025

@rkazants, please check out this PR

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 80.16%. Comparing base (decd6ba) to head (6abcd2b).
Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
keras/src/backend/openvino/numpy.py 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21017      +/-   ##
==========================================
- Coverage   82.45%   80.16%   -2.30%     
==========================================
  Files         562      564       +2     
  Lines       53720    54123     +403     
  Branches     8335     8412      +77     
==========================================
- Hits        44297    43386     -911     
- Misses       7381     8717    +1336     
+ Partials     2042     2020      -22     
Flag Coverage Δ
keras 79.98% <0.00%> (-2.30%) ⬇️
keras-jax 64.03% <0.00%> (+0.38%) ⬆️
keras-numpy 59.11% <0.00%> (+0.50%) ⬆️
keras-openvino ?
keras-tensorflow 64.35% <0.00%> (+0.26%) ⬆️
keras-torch 64.07% <0.00%> (+0.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rkazants
Copy link
Contributor

rkazants commented Mar 13, 2025

a lot of unneeded changes:
{0C31B750-BFC2-47B5-814A-EA1809765F09}

Please fix it.

@Hmm-1224
Copy link
Author

Hmm-1224 commented Mar 13, 2025

check if it is fine now.. i changed the branch

@Hmm-1224
Copy link
Author

hello @rkazants,
please do check my update and guide me further if needed...

Comment on lines 767 to 770
if not isinstance(xs, (list, tuple)):
raise TypeError("Input to `hstack` must be a list or tuple of tensors.")
if len(xs) == 0:
raise ValueError("Input list to `hstack` cannot be empty.")
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove, not needed checks. TF backend does not have such check for hstack implementation.

Comment on lines 767 to 770
if not isinstance(xs, (list, tuple)):
raise TypeError("Input to `hstack` must be a list or tuple of tensors.")
if len(xs) == 0:
raise ValueError("Input list to `hstack` cannot be empty.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not isinstance(xs, (list, tuple)):
raise TypeError("Input to `hstack` must be a list or tuple of tensors.")
if len(xs) == 0:
raise ValueError("Input list to `hstack` cannot be empty.")

element_type = x.output.get_element_type()
break
xs = [get_ov_output(x, element_type) for x in xs]
xs = _align_operand_types(xs[0], xs[1], "hstack()")
Copy link
Contributor

Choose a reason for hiding this comment

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

here you align only first two elements but it does not cover a case of list with three elements and more. Please align all of them to xs[0] and then call concat

@Hmm-1224 Hmm-1224 closed this Mar 18, 2025
@Hmm-1224 Hmm-1224 deleted the main branch March 18, 2025 04:53
@Hmm-1224 Hmm-1224 restored the main branch March 24, 2025 18:42
@Hmm-1224 Hmm-1224 reopened this Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Assigned Reviewer
Development

Successfully merging this pull request may close these issues.

4 participants