-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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", |
There was a problem hiding this comment.
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 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* max parameters . | |
* max parameters. |
} | ||
const content = fs.readFileSync(inputFile).toString(); | ||
const jsonDict = JSON.parse( | ||
content.replace(/\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g, |
There was a problem hiding this comment.
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?
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 || |
There was a problem hiding this comment.
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 && |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
Just noticed this is still open. Is it obviated by other changes? |
Should we complete or abandon this? |
This PR includes
softsign
andgelu
operators.@huningxin @fdwr PTAL, thanks.