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

ScatterND's doc gives wrong result for 3d indice case #6019

Open
haowang5128 opened this issue Mar 13, 2024 · 2 comments
Open

ScatterND's doc gives wrong result for 3d indice case #6019

haowang5128 opened this issue Mar 13, 2024 · 2 comments
Labels
bug contributions welcome documentation Issues related to ONNX documentation spec clarification Clarification of the ONNX spec needed
Milestone

Comments

@haowang5128
Copy link

Bug Report

Describe the bug

In ScatterND doc , it shows scatternd computation as:

output = np.copy(data)
update_indices = indices.shape[:-1]
for idx in np.ndindex(update_indices):
    output[indices[idx]] = f(output[indices[idx]], updates[idx])

For a 3d indices case like this:

input = [[[0., 1.],  [2., 3.]], [[4., 5.], [6., 7.]]]
indices = [[[0, 0]]]
updates = [[[100., 101.]]]
reduction  = None

It provides results different from ONNXRuntime.

  • The doc's method: [[[100., 101.], [100., 101.]], [[ 4., 5.], [ 6., 7.]]]
  • ONNXRuntime: [[[100., 101.], [ 2., 3.]], [[ 4., 5.], [ 6., 7.]]]

In the doc's method, indices[idx] gives a numpy array, output[indices[idx]] will not select the correct element. Convert indices[idx] to tuple will fix this.

output = np.copy(data)
update_indices = indices.shape[:-1]
for idx in np.ndindex(update_indices):
    output[tuple(indices[idx])] = f(output[tuple(indices[idx])], updates[idx])
@haowang5128 haowang5128 changed the title ScatterND doc ScatterND's doc gives wrong result for 3d indice case Mar 13, 2024
@justinchuby justinchuby added documentation Issues related to ONNX documentation contributions welcome labels Mar 14, 2024
@liqunfu
Copy link
Contributor

liqunfu commented Mar 16, 2024

@haowang5128 , thank you for pointing out this. Would you like to create a PR for the fix. If so, please also correct sample implementation at:

output[tuple(indices[i])] = np.maximum(output[indices[i]], updates[i])
. The code does produce correct test data. However, to have extra tuple at the right hand side makes more sense.

@xadupre
Copy link
Contributor

xadupre commented Apr 9, 2024

Is it on CPU or GPU? This PR addresses one issue on GPU: microsoft/onnxruntime#19540.

@justinchuby justinchuby added the spec clarification Clarification of the ONNX spec needed label Apr 11, 2024
@justinchuby justinchuby added this to the 1.17 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contributions welcome documentation Issues related to ONNX documentation spec clarification Clarification of the ONNX spec needed
Projects
None yet
Development

No branches or pull requests

4 participants