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

Generate baseline data script for operator with single input #78

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

BruceDai
Copy link
Contributor

@BruceDai BruceDai commented May 29, 2024

This PR includes

  1. generate baseline data script
  2. baseline data for softsign and gelu operators.

@huningxin @fdwr PTAL, thanks.

@BruceDai BruceDai changed the title Gen softsign baseline Generate baseline data for single input May 29, 2024
@BruceDai BruceDai changed the title Generate baseline data for single input Generate baseline data script for operator with single input May 29, 2024
Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

Thanks B.

```

then, you can find two generated folders named 'test-data' and
'test-data-wpt'. There're raw test data as being
Copy link

Choose a reason for hiding this comment

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

They're?

node gen-operator-with-single-input.js resources\softsign.json
```

then, you can find two generated folders named 'test-data' and
Copy link

Choose a reason for hiding this comment

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

Then you ...

};
const inputTensor = new Tensor(input.shape, input.data);
const outputTensor =
operatorMappingDict[operatorName](inputTensor, options);
Copy link

Choose a reason for hiding this comment

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

(minor style) Hmm, code is more readable when it's not awkwardly split like this for going just one column past 80.

    const inputTensor = new Tensor(input.shape, input.data);
    const outputTensor = operatorMappingDict[operatorName](inputTensor, options);

There's plenty of screen space on monitors in the year 2024, and I'm curious why 80 is being applied here when the root eslint is set to 100 columns? https://github.com/webmachinelearning/webnn-baseline/blob/main/.eslintrc.js#L17 In any case, it seems inconsistent.

"inputs": {
"input": {
"shape": [2, 1, 4, 1, 3],
"data": "float64Data",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"data": "float64Data",
"data": "float16Data",

Typo?

The output below is float16Data.

      "expected": {
        "name": "output",
        "shape": [2, 1, 4, 1, 3],
        "data": "float16Data",
        "type": "float16"
      }

"expected": {
"name": "output",
"shape": [24],
"data": "float321DNegativeDefault",
Copy link

@fdwr fdwr Jun 7, 2024

Choose a reason for hiding this comment

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

floating point 321-dimensional data? 😉 Propose putting a separator between them, like "float32-1D" or "float32_1D" for some clarity.


/**
* Prepare input data by specified config of inputsDataInfo, dataFile, min,
* max parameters .
Copy link

Choose a reason for hiding this comment

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

Suggested change
* max parameters .
* max parameters.

}
const content = fs.readFileSync(inputFile).toString();
const jsonDict = JSON.parse(
content.replace(/\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g,
Copy link

Choose a reason for hiding this comment

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

What is this regex doing? Is it removing comments?

Suggested change
content.replace(/\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g,
content.replace(/\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g, // remove comments

// If found, it replaces it by a trio
if ( value instanceof Int8Array ||
value instanceof Uint8Array ||
value instanceof Uint16Array ||
Copy link

Choose a reason for hiding this comment

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

<minor>Should we add Int16Array here right now for consistency with Uint16Array? WebNN doesn't support either int16 or uint16 currently, but if it does in the future, that's one less place in the test code to find a bug later. Though I suppose you are probably adding this for the sake of float16.</minor>

value instanceof Float32Array ||
value instanceof Float16Array ||
value instanceof Float64Array ) {
if (value.length ===1) { // value instanceof Uint8Array &&
Copy link

Choose a reason for hiding this comment

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

&&?

const result = [];
result[0] = value[0];
return result;
} else {
Copy link

Choose a reason for hiding this comment

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

Is this the BigInt64Array case? If so, comment is nice.

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM % Dwayne's comments, Thanks!

@fdwr
Copy link

fdwr commented Jul 29, 2024

Just noticed this is still open. Is it obviated by other changes?

@fdwr
Copy link

fdwr commented Sep 9, 2024

Should we complete or abandon this?

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

Successfully merging this pull request may close these issues.

3 participants