Skip to content

Support numpy.moveaxis operation #21189

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

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

Conversation

Prabha14039
Copy link

@Prabha14039 Prabha14039 commented Apr 20, 2025

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.59%. Comparing base (dd9132f) to head (8e8bdc5).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #21189   +/-   ##
=======================================
  Coverage   82.59%   82.59%           
=======================================
  Files         564      564           
  Lines       54407    54428   +21     
  Branches     8449     8454    +5     
=======================================
+ Hits        44936    44957   +21     
  Misses       7396     7396           
  Partials     2075     2075           
Flag Coverage Δ
keras 82.40% <100.00%> (+<0.01%) ⬆️
keras-jax 63.78% <0.00%> (-0.03%) ⬇️
keras-numpy 58.90% <0.00%> (-0.02%) ⬇️
keras-openvino 32.97% <100.00%> (+0.03%) ⬆️
keras-tensorflow 64.19% <0.00%> (-0.03%) ⬇️
keras-torch 63.87% <0.00%> (-0.03%) ⬇️

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.

@Prabha14039 Prabha14039 marked this pull request as ready for review April 20, 2025 19:56
Comment on lines +1041 to +1045
for axis in sorted(source, reverse=True):
axes.pop(axis)

for dest, src in sorted(zip(destination, source)):
axes.insert(dest, src)
Copy link
Contributor

@rkazants rkazants Apr 21, 2025

Choose a reason for hiding this comment

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

I think it is more clear to code this as:

for dest, src in sorted(zip(destination, source)):
    axes[dest] = src

Copy link
Author

Choose a reason for hiding this comment

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

i dont think we can do this because this results in broken permuation
as this assumes axes already has the correct length

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share a concrete example that shows it will not work?

Copy link
Author

Choose a reason for hiding this comment

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

image

This is happening because the following code assumes that the index is of equal length
but after popping the elements the length is not the same hence it results in index error if dest >= len(axes)

def moveaxis(x, source, destination):

    if isinstance(source, int):
        source = [source]
    if isinstance(destination, int):
        destination = [destination]

    x = get_ov_output(x)
    x_shape = x.get_partial_shape()
    rank = x_shape.rank.get_length()

    source = [s + rank if s < 0 else s for s in source]
    destination = [d + rank if d < 0 else d for d in destination]

    axes = list(range(rank))

    for axis in sorted(source, reverse=True):
        axes.pop(axis)

    for dest, src in sorted(zip(destination, source)):
        axes[dest] = src

    perm = ov_opset.constant(axes, dtype=Type.i32).output(0)

    x_type = x.get_element_type()
    if x_type == Type.bf16:
        x = ov_opset.convert(x, Type.f32).output(0)
        result = ov_opset.transpose(x, perm).output(0)
        return OpenVINOKerasTensor(
            ov_opset.convert(result, Type.bf16).output(0)
        )

    return OpenVINOKerasTensor(ov_opset.transpose(x, perm).output(0))

Copy link
Contributor

Choose a reason for hiding this comment

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

@Prabha14039, once again. Share with me a concrete example with concrete values source and destination for which my code willn't work.

Copy link
Author

Choose a reason for hiding this comment

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

i apologise here is the test case failing
image

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to remove as well:
{BB02C2C5-31A5-4E32-8BED-46BC31665DA8}

Copy link
Author

Choose a reason for hiding this comment

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

image

i am facing the following issue after removing the axes.pop

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I don't see source and destination values. Please create standlalone code that proves my code does not work.

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.

[Good First Issue][Keras 3 OpenVINO Backend]: Support numpy.moveaxis operation
4 participants