Use ULP based comparison. #139

BruceDai · 2021-11-10T07:32:27Z

int64_t GetBitwise(float value) {
    int64_t bitwiseValue = (value < T(0)) ? ~int64_t(0) : 0; // Extend sign.
    *std::launder(reinterpret_cast<T*>(&bitwiseValue)) = value;
    return bitwiseValue;
}

bool CompareUlp(float a, float b, uint64_t ulp) {
    return static_cast<uint64_t>(abs(GetBitwise(a) - GetBitwise(b))) > ulp;
}

@huningxin I implemented JavaScript compareUlp() function to align Chai's comment on #2, PTAL, thanks.

huningxin · 2021-11-10T07:37:50Z

test/utils.js

@@ -36,11 +36,28 @@ export function almostEqual(a, b, criteria) {
  }
 }

-export function checkValue(
-    output, expected, criteria = opFp32AccuracyCriteria) {
+export function compareUlp(a, b, nulp = 1, base = 64) {


Please split this function into getBitwise and cmopareUlp as Chai's C++ example.

For getBitwise, please change base to a string type, like float32 and float64. BTW, I don't think we support float64 in spec so far, do we?

huningxin · 2021-11-10T07:39:19Z

test/utils.js

+  const bView = new DataView(bBuffer);
+
+  if (base === 64) {
+    aView.setFloat64(0, a, true);


I suppose we should not restrict to little endian.

huningxin · 2021-11-10T07:47:42Z

test/utils.js

+export function compareUlp(a, b, nulp = 1, base = 64) {
+  const aBuffer = new ArrayBuffer(8);
+  const bBuffer = new ArrayBuffer(8);
+  const aView = new DataView(aBuffer);


It should populate the buffer with 0 or ~0, as C++ code does

int64_t bitwiseValue = (value < T(0)) ? ~int64_t(0) : 0; // Extend sign.

You probably can use a JavaScript BigInt object do that e.g.,

aView.setBigInt64(0, value < 0 ? ~BigInt(0) : BigInt(0))

test/utils.js

huningxin · 2021-11-10T07:48:56Z

test/utils.js

+    bView.setFloat32(0, b, true);
+  }
+
+  return Math.abs(new BigUint64Array(aBuffer) - new BigUint64Array(bBuffer)) >


For getBitwise, you could return aView.getBigInt64(0) here.

Math.abs doesn't work with BigInt64, so you probably need to implement it by yourself. Fortunately, it would straightfoward, as BigInt64 support comparison and negate.

BruceDai · 2021-11-10T09:45:07Z

Thanks @huningxin .

I updated PR following your suggestion, now new getBitwise() function is only for float32 data

function getBitwise(value) {
  const buffer = new ArrayBuffer(4);
  const view = new DataView(buffer);
  view.setFloat32(0, value);
  return view.getInt32(0);
}

About bitwise of float32 data, I compared the return value of this JS function with one of native C++ GetBitwise() function, they are same.

@huningxin Please take a look again, thanks.

huningxin · 2021-11-17T01:15:10Z

test/utils.js

-export function checkValue(
-    output, expected, criteria = opFp32AccuracyCriteria) {
+function getBitwise(value) {
+  const buffer = new ArrayBuffer(4);


Should use length 8 to accommodate BigInt

huningxin · 2021-11-17T01:16:32Z

test/utils.js

+  const buffer = new ArrayBuffer(4);
+  const view = new DataView(buffer);
+  view.setFloat32(0, value);
+  return view.getInt32(0);


should use getBigInt64 and return a BigInt as I commented #139 (comment)

huningxin · 2021-11-17T01:18:20Z

test/utils.js

-    output, expected, criteria = opFp32AccuracyCriteria) {
+function getBitwise(value) {
+  const buffer = new ArrayBuffer(4);
+  const view = new DataView(buffer);


as I commented before, you should populate the buffer #139 (comment)

huningxin · 2021-11-17T01:19:19Z

test/utils.js

+  return view.getInt32(0);
+}
+
+export function compareUlp(a, b, nulp = 1, format = 'float32') {


nulp should be BigInt

huningxin · 2021-11-17T01:20:08Z

test/utils.js

+}
+
+export function compareUlp(a, b, nulp = 1, format = 'float32') {
+  assert(format === 'float32', `Format ${format} is not supported.`);


format is not a accurate name, probably dataType

huningxin · 2021-11-17T01:21:41Z

test/utils.js

@@ -36,11 +36,26 @@ export function almostEqual(a, b, criteria) {
  }
 }

-export function checkValue(
-    output, expected, criteria = opFp32AccuracyCriteria) {
+function getBitwise(value) {


If it is only for float32, please name it as getBitwiseFloat32

huningxin · 2021-11-17T01:22:42Z

test/utils.js

+  const aBitwise = getBitwise(a);
+  const bBitwise = getBitwise(b);
+  let distance = aBitwise - bBitwise;
+  distance = distance > 0 ? distance : -distance;


BruceDai · 2021-11-17T09:24:06Z

Thanks much @huningxin.
I've updated commit to fix your review comments, PTAL, Thanks.

huningxin

Please also add js doc for these two methods. Thanks.

huningxin · 2021-11-18T00:36:34Z

test/utils.js

@@ -36,11 +36,28 @@ export function almostEqual(a, b, criteria) {
  }
 }

-export function checkValue(
-    output, expected, criteria = opFp32AccuracyCriteria) {
+export function getBitwiseFloat32(value) {


It probably makes sense to support other data types and make this function more generic, like getBitwise. It would also align with C++ implementation.

huningxin · 2021-11-18T00:37:00Z

test/utils.js

+  const buffer = new ArrayBuffer(8);
+  const int64Array = new BigInt64Array(buffer);
+  int64Array[0] = value < 0 ? ~BigInt(0) : BigInt(0);
+  const f32Array = new Float32Array(buffer);


Create the typed array based on dataType.

You can assert the unsupported data types here.

huningxin · 2021-11-18T00:37:59Z

test/utils.js

+}
+
+export function compareUlp(a, b, nulp = 1n, dataType = 'float32') {
+  assert(dataType === 'float32', `DataType ${dataType} is not supported.`);


avoid assertion here.

BruceDai · 2021-11-18T03:52:34Z

@huningxin I updated PR, PTAL, thanks.

huningxin

Looks good, thanks @BruceDai .

BruceDai · 2021-11-18T10:45:42Z

test/utils.js

@@ -45,6 +45,9 @@ export function almostEqual(a, b, criteria) {
 * @return {number} A 64-bit signed integer.
 */
 export function getBitwise(value, dataType) {
+  if (Object.is(value, -0)) {


@huningxin I caught an issue of -0, the returns of

getBitwise(-0, "float32")

is 2147483648n, so the distance between -0 and 0 is 2147483648n, it isn't the same as the distance of 0n between -0 and 0 by native C++ compareUlp() function.

I added this workaround to deal with -0. What's your opinion? Thanks

Did you try -0.0 in native C++?

Hi @huningxin,
I tested with -0.0 in native C++, the GitBitwise() function returns 2147483648n, and the distance between -0.0 and 0 is 2147483648n.
I checked that return of Object.is(-0.0, -0) is ture, that's, -0.0 and -0 are same value in JavaScript.
Should we make the behave in JavaScript align with native behave? Any suggestion? Thanks.

Thanks for the testing. I think in JavaScript -0 and -0.0 are same, because they are both Numbers. In C++, -0 is integer and -0.0 is float, so they are different.

With that, I think it doesn't need this conversion with float32 data type. For integer data type, it probably needs to use parseInt(v), it will convert -0 (float) to 0.

Thanks @huningxin.
I will raise this special case on coming WG meeting.

Cool. Could you please file an issue to log this case? It could be used for WG discussion. Thanks.

…ents.

1. Make getBitwise() function more generic 2. Add JS docs for getBitwise() and compareUlp() functions

BruceDai · 2021-11-30T08:14:19Z

I rebased this PR to only add JavaScript's ULP based comparison implementation.

@huningxin PTAL, thanks.

huningxin reviewed Nov 10, 2021

View reviewed changes

BruceDai force-pushed the ulp_based_comparision branch from 8b7c0e2 to 98d76cb Compare November 10, 2021 09:35

BruceDai force-pushed the ulp_based_comparision branch from 98d76cb to 232887b Compare November 10, 2021 09:50

huningxin requested changes Nov 17, 2021

View reviewed changes

huningxin reviewed Nov 18, 2021

View reviewed changes

huningxin approved these changes Nov 18, 2021

View reviewed changes

BruceDai commented Nov 18, 2021

View reviewed changes

BruceDai added 4 commits November 30, 2021 16:07

Add JavaScript ULP based comparison.

cf09512

Use TypedArray to implement getBitwise() function and fix review comm…

4447615

…ents.

Fix lint issue "'BigInt' is not defined".

d5756c9

Address review comments:

a111503

1. Make getBitwise() function more generic 2. Add JS docs for getBitwise() and compareUlp() functions

BruceDai force-pushed the ulp_based_comparision branch from db9b729 to a111503 Compare November 30, 2021 08:09

huningxin merged commit e89a663 into webmachinelearning:master Nov 30, 2021

BruceDai mentioned this pull request Dec 1, 2021

Max ULP distance for WebNN ops with pr #139 - ULP comparison #144

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ULP based comparison. #139

Use ULP based comparison. #139

BruceDai commented Nov 10, 2021

huningxin Nov 10, 2021

huningxin Nov 10, 2021

huningxin Nov 10, 2021

huningxin Nov 10, 2021

huningxin Nov 10, 2021

BruceDai commented Nov 10, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

huningxin Nov 17, 2021

BruceDai commented Nov 17, 2021

huningxin left a comment

huningxin Nov 18, 2021

huningxin Nov 18, 2021

huningxin Nov 18, 2021

huningxin Nov 18, 2021

BruceDai commented Nov 18, 2021

huningxin left a comment

BruceDai Nov 18, 2021

huningxin Nov 18, 2021

BruceDai Nov 18, 2021

huningxin Nov 19, 2021

BruceDai Nov 30, 2021

huningxin Nov 30, 2021

BruceDai commented Nov 30, 2021

Use ULP based comparison. #139

Use ULP based comparison. #139

Conversation

BruceDai commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceDai commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceDai commented Nov 17, 2021

huningxin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceDai commented Nov 18, 2021

huningxin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceDai commented Nov 30, 2021