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

[OpenVINO backend]: Support numpy.bincount #20940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

11happy
Copy link

@11happy 11happy commented Feb 22, 2025

Overview:

  • This PR implements support for numpy.bincount operation

Testing:

  • Ensured all tests are passing & other functionalities remain unaffected.

CC:

@11happy
Copy link
Author

11happy commented Feb 22, 2025

Here there are two things I am not sure about :

  • sparse arguement I did not find its documentation on the official numpy docs
    image

  • Here while getting the max_element using reduce_max op I have set the reduction_axes=[0] assuimg 1D input tensor as mentioned in numpy docs

Thank you

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2025

Codecov Report

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

Project coverage is 71.23%. Comparing base (86dce6f) to head (2b44541).

Files with missing lines Patch % Lines
keras/src/backend/openvino/numpy.py 0.00% 25 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (86dce6f) and HEAD (2b44541). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (86dce6f) HEAD (2b44541)
keras 5 3
keras-tensorflow 1 0
keras-jax 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #20940       +/-   ##
===========================================
- Coverage   82.43%   71.23%   -11.21%     
===========================================
  Files         561      561               
  Lines       53205    53230       +25     
  Branches     8242     8245        +3     
===========================================
- Hits        43862    37918     -5944     
- Misses       7336    13331     +5995     
+ Partials     2007     1981       -26     
Flag Coverage Δ
keras 71.12% <0.00%> (-11.14%) ⬇️
keras-jax ?
keras-numpy 58.82% <0.00%> (-0.03%) ⬇️
keras-openvino 32.55% <0.00%> (-0.07%) ⬇️
keras-tensorflow ?
keras-torch 64.06% <0.00%> (-0.04%) ⬇️

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.

@11happy
Copy link
Author

11happy commented Feb 23, 2025

Hii :)
could you please help me how to resolve this issue, thank you

FAILED keras/src/ops/numpy_test.py::NumpyDtypeTest::test_bincount_uint32 - ValueError: unsupported type of `x` to create ov.Output: <class 'NoneType'>

@nikolasavic3
Copy link
Contributor

have you tried testing locally? Does it work for you?
you can do:
pytest keras/src/ops/numpy_test.py::NumpyDtypeTest::test_bincount_int16

@11happy
Copy link
Author

11happy commented Feb 23, 2025

have you tried testing locally? Does it work for you? you can do: pytest keras/src/ops/numpy_test.py::NumpyDtypeTest::test_bincount_int16

yes everytime locally i run this command
pytest -c ./pytest.ini ./keras/src/ops/numpy_test.py
& its passing

@nikolasavic3
Copy link
Contributor

nikolasavic3 commented Feb 23, 2025

yes everytime locally i run this command pytest -c ./pytest.ini ./keras/src/ops/numpy_test.py & its passing

I had the same problem.
I wrote this in the terminal and after that tests started failing locally too:
export KERAS_BACKEND=openvino
and this if you don't have a GPU:

export JAX_PLATFORMS=cpu
export XLA_FLAGS=--xla_gpu_cuda_data_dir=""

You can then test like this:
pytest keras/src/ops/numpy_test.py
or this:
pytest keras/src/ops/numpy_test.py::NumpyDtypeTest::test_bincount_int16

@nikolasavic3
Copy link
Contributor

if you want to fix a mistake in a file without doing a new commit you can do:

git add keras/src/backend/openvino/numpy.py # after you fix the mistake in the file
git commit --amend --no-edit # you can commit having all changes within the last commit
git push --force-with-lease

@11happy
Copy link
Author

11happy commented Feb 23, 2025

if you want to fix a mistake in a file without doing a new commit you can do:

git add keras/src/backend/openvino/numpy.py # after you fix the mistake in the file git commit --amend --no-edit # you can commit having all changes within the last commit git push --force-with-lease

thanks for constantly helping me on this PR, I didnt knew about --force-with-lease I will try it next time, I currenlty did rebase while squashing all commits & force pushed it.

@11happy 11happy force-pushed the numpy.bincount branch 2 times, most recently from fc1dac7 to ac62d35 Compare February 23, 2025 16:53
rebased

fix: hardcode dtype int32 when weights=none

Signed-off-by: 11happy <[email protected]>

fix: use np.expand_dims

Signed-off-by: 11happy <[email protected]>

remove unecessary headers

Signed-off-by: 11happy <[email protected]>

style: reformat numpy_test.py

Signed-off-by: 11happy <[email protected]>
@11happy
Copy link
Author

11happy commented Feb 28, 2025

Gentle Ping! @fchollet , @rkazants .
I have fixed the merge conflicts & openvino CI issues.
Thank you

Comment on lines -3582 to +3583
x = knp.expand_dims(x, 0)
weights = knp.expand_dims(weights, 0)
x = np.expand_dims(x, 0)
weights = np.expand_dims(weights, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert these changes

Copy link
Author

Choose a reason for hiding this comment

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

Earlier the openvino CI was failing because of this as in the implementation of expand_dims if input is not an instance of openvinokerastensor it was going in else statement causing a false assert.

@@ -95,6 +96,7 @@ NumpyOneInputOpsCorrectnessTest::test_argmin
NumpyOneInputOpsCorrectnessTest::test_argpartition
NumpyOneInputOpsCorrectnessTest::test_argsort
NumpyOneInputOpsCorrectnessTest::test_array
NumpyOneInputOpsCorrectnessTest::test_average
NumpyOneInputOpsCorrectnessTest::test_bincount
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 should only remove lines corresponding to bincount tests.
Now you are adding for average for some reason

Copy link
Author

Choose a reason for hiding this comment

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

sorry, while resolving the merge conflict I took a merge with the master branch and it was an incoming change so I did kept a combination of both current and incoming, I will remove it.
Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants