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

Completed cumulativeSum #103

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions src/cumulativeSum.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

import {Tensor, sizeOfShape} from './lib/tensor.js';
import {validateCumulativeSumParams} from './lib/validate-input.js';

/**
* Computes the cumulative sum of the input tensor along the specified axis.
* @param {Tensor} input
* @param {number} axis
* @param {MLCumulativeSumOptions} options
* @return {Tensor}
*/
export function cumulativeSum(input, axis, {exclusive = 0, reverse = 0} = {}) {
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
export function cumulativeSum(input, axis, {exclusive = 0, reverse = 0} = {}) {
export function cumulativeSum(input, axis, {exclusive = false, reverse = false} = {}) {

Refer to https://chromium-review.googlesource.com/c/chromium/src/+/5845069/9/third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.idl#66

dictionary MLCumulativeSumOptions : MLOperatorOptions {
    boolean exclusive = false;
    boolean reversed = false;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

validateCumulativeSumParams(...arguments);
const inputShape = input.shape;
const outputShape = [...inputShape];
Copy link

Choose a reason for hiding this comment

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

Suggested change
const outputShape = [...inputShape];
const outputShape = inputShape;

Do we need the [... ], since outputShape will always equal inputShape anyway (no modification)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const output = new Tensor(outputShape);
const numElementsAlongAxis = inputShape[axis];

const totalElements = sizeOfShape(outputShape);

for (let outputIndex = 0; outputIndex < totalElements; outputIndex++) {
Copy link

@fdwr fdwr Sep 25, 2024

Choose a reason for hiding this comment

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

It appears this nested for loop repeats the same computation for the same output axial sliver, overwriting the same output elements multiple times. I recommend skipping it if it's already computed in an earlier loop. e.g.

const elementStart = reverse ? elementCountAlongAxis - 1 : 0;
const elementStep = reverse ? -1 : 1;

for (let outputIndex = 0; outputIndex < totalElements; ++outputIndex) {
  let location = output.locationFromIndex(outputIndex);
  if (location[axis] > 0) {
    continue; // No need to compute this axis again, since it was already done.
  }

  let cumulativeSumValue = 0;

  // Compute the accumulated sum along this entire axis.
  for (let i = 0; i < elementCountAlongAxis; ++i) {
    location[axis] = elementStart + i * elementStep;

    const inputValue = input.getValueByLocation(inputLocation);
    const oldCumulativeSumValue = cumulativeSumValue;
    cumulativeSumValue += inputValue;
    const outputValue = exclusive ? oldCumulativeSumValue : cumulativeSumValue;
    output.setValueByLocation(outputLocation, outputValue);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

Choose a reason for hiding this comment

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

I'm still seeing the doubly nested loop such that the inner loop redundantly overwrites the previous loop's output (which will be identical each time for a given summation sliver). Notice the continue statement above, since we only need to compute the sum (inner loop) along the axis once for each distinct sliver (outer loop). Also, I since realized that the input and the output locations are always identical, meaning we can fold them into a single location variable. Updated above accordingly.

const loc = output.locationFromIndex(outputIndex);
Copy link

Choose a reason for hiding this comment

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

request loc -> location (whole words policy helps readability for others). Same for inputLocation and outputLocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

let cumulativeSumValue = 0;

const start = reverse ? numElementsAlongAxis - 1 : 0;
const step = reverse ? -1 : 1;
const end = reverse ? -1 : numElementsAlongAxis;
Copy link

Choose a reason for hiding this comment

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

These three values never change and can be computed once outside the loop.


for (let i = start; reverse ? i > end : i < end; i += step) {
Copy link

Choose a reason for hiding this comment

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

Since we are guaranteed this loop will end (given step and end are selected accordingly above, which they are), this...

reverse ? i > end : i < end

...could just be...

i != end

Alternately (and probably clearer) we could make i always be relative to the output offset and use <, computing the input coordinate using the step.

  for (let i = 0; i < elementCountAlongAxis; ++i) {
    inputLocation[axis] = inputElementStart + i * inputElementStep;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const inputLoc = [...loc];
inputLoc[axis] = exclusive ? (reverse ? i + 1 : i - 1) : i;

if (!exclusive || (exclusive && inputLoc[axis] >= 0 &&
inputLoc[axis] < numElementsAlongAxis)) {
cumulativeSumValue += input.getValueByLocation(inputLoc);
}

const outputLoc = [...loc];
outputLoc[axis] = i;
output.setValueByLocation(outputLoc, cumulativeSumValue);
}
}

return output;
}
12 changes: 12 additions & 0 deletions src/lib/validate-input.js
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,18 @@ export function validateGatherParams(input, indices, {axis = 0} = {}) {
}
}

export function validateCumulativeSumParams(input, axis) {
if (axis !== undefined) {
const rank = input.rank;
if (!Number.isInteger(axis) || axis < -rank || axis >= rank) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The axis is of unsigned long, so its range is in [0, rank). Please modify this check and delete the tests with negative axis, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

Choose a reason for hiding this comment

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

Yeah, we generally want WebNN to be more explicit, resolving these kinds of user-facing API conveniences (like negative numbers and output shape rounding) into lower-level concrete values, like the actual axis and specific output shape.

throw new Error(`The axis ${axis} should be in the range [-rank(input), rank(input)-1].`);
}
if (axis >= rank) {
throw new Error(`The axis ${axis} should be in the interval [0, ${rank}).`);
Copy link

Choose a reason for hiding this comment

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

Hmm, could just combine these two.

    if (!Number.isInteger(axis) || axis < 0 || axis >= rank) {
      throw new Error(`The axis ${axis} must be an unsigned integer in the interval [0, ${rank}).`);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
}
}

export function validateTriangularParams(input, {diagonal = 0} = {}) {
const inputRank = input.rank;
if (inputRank < 2) {
Expand Down
143 changes: 143 additions & 0 deletions test/cumulativeSum_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
'use strict';

import {cumulativeSum} from '../src/cumulativeSum.js';
import {Tensor} from '../src/lib/tensor.js';
import * as utils from './utils.js';

describe('test cumulativeSum', function() {
function testCumulativeSum(input, axis, options={}, expected) {
const tensor = new Tensor(input.shape, input.data);
const outputTensor = cumulativeSum(tensor, axis, options);
console.log('outputTensor', outputTensor);
utils.checkShape(outputTensor, expected.shape);
utils.checkValue(outputTensor, expected.data);
}

it('test cumulativeSum 1d', function() {
const input = {
shape: [5],
data: [
1, 2, 3, 4, 5,
],
};
const axis=0;
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
const axis=0;
const axis = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const options = {exclusive: 0, reverse: 0};
const expected = {
shape: [5],
data: [
1, 3, 6, 10, 15,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 1d exclusive', function() {
const input = {
shape: [5],
data: [
1, 2, 3, 4, 5,
],
};
const axis=0;
const options = {exclusive: 1, reverse: 0};
const expected = {
shape: [5],
data: [
0, 1, 3, 6, 10,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 1d reverse', function() {
const input = {
shape: [5],
data: [
1, 2, 3, 4, 5,
],
};
const axis=0;
const options = {exclusive: 0, reverse: 1};
const expected = {
shape: [5],
data: [
15, 14, 12, 9, 5,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 1d reverse exclusive', function() {
const input = {
shape: [5],
data: [
1, 2, 3, 4, 5,
],
};
const axis=0;
const options = {exclusive: 1, reverse: 1};
const expected = {
shape: [5],
data: [
14, 12, 9, 5, 0,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 2d', function() {
const input = {
shape: [2, 3],
data: [
1, 2, 3, 4, 5, 6,
],
};
const axis=0;
const options = {exclusive: 0, reverse: 0};
const expected = {
shape: [2, 3],
data: [
1, 2, 3, 5, 7, 9,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 2d axis=1', function() {
const input = {
shape: [2, 3],
data: [
1, 2, 3, 4, 5, 6,
],
};
const axis=1;
const options = {exclusive: 0, reverse: 0};
const expected = {
shape: [2, 3],
data: [
1, 3, 6, 4, 9, 15,
],
};
testCumulativeSum(input, axis, options, expected);
});

it('test cumulativeSum 2d negtive axis', function() {
const input = {
shape: [2, 3],
data: [
1, 2, 3, 4, 5, 6,
],
};
const axis=1;
const options = {exclusive: 0, reverse: 0};
const expected = {
shape: [2, 3],
data: [
1, 3, 6, 4, 9, 15,
],
};
testCumulativeSum(input, axis, options, expected);
});
});


Loading