Add assertEqual #457

dangerdak · 2018-03-29T15:19:38Z

I attempted to address #423 , adding assertEqual to util.ts and sharing its code with tensor_util.ts and jasmine_shim.ts
Any guidance or feedback would be great @ry @piscisaureus

ry

Awesome work! Thank you!
Looks good modulo some minor comments.
@piscisaureus please review this due to the sensitive nature of the patch.

ry · 2018-03-29T15:39:09Z

src/dataset_test.ts

+  assertAllEqual,
+  assertEqual,
+  assertShapesEqual
+}
  from "./tensor_util";


Style nit: Please have the closing curly brace and from "./tensor_util"; on the same line.

ry · 2018-03-29T15:41:35Z

src/dl/util.ts

@@ -17,6 +17,7 @@ import { DataType, DataTypeMap, NDArray } from "./math/ndarray";
 * =============================================================================
 */

+export { assertEqual } from "../util";


Although this might have been violated elsewhere, the intention is that src/dl is standalone and imports should not reach outside of it. Please remove.

ry · 2018-03-29T15:53:21Z

tools/jasmine_shim.ts

+    try {
+      assertEqual(value, expected);
+      return true;
+    } catch (e) {


Why is the try catch needed?

I put it there because assertEqual throws if its arguments aren't equal, whereas toEqual returns a boolean

Ah makes sense - In that case can you add another routine to util.ts called equal which returns boolean, and use that here and in assertEqual, so the try block can be avoided?

ry · 2018-03-29T15:54:28Z

src/util.ts

+      return true;
+    }
+    if (a && typeof a === "object" && b && typeof b === "object") {
+      if (seen.get(a) === b) {


In this file there is a method called objectsEqual - can that be reused here?

Great, thanks. Am I right in thinking that assertObjectsEqual (https://github.com/propelml/propel/blob/master/src/util.ts#L140) isn't needed anymore as assertEqual deals with objects?

Yes. Please replace assertObjectsEqual calls with asserEqual

I tried using objectsEqual but it doesnt work with nested objects so I kept equal as it is here (and replaced objectsEqual with equal)

great - thank you!

ry · 2018-03-29T15:56:08Z

src/util.ts

+export function assertEqual(actual: any, expected: any, msg = null) {
+  if (!msg) { msg = `actual: ${actual} expected: ${expected}`; }
+  if (!equal(actual, expected)) {
+    throw new Error(msg);


For debugging on web, in the case where the actual and expected are objects - it would be helpful to have an additional output here. Something like this:

console.error("assertEqual failed. actual = ", actual, "expected =", expected)

(This way the nice inspector interface can be used to inspect deep objects)

ry · 2018-03-29T22:43:58Z

src/dl/util.ts

+        return false;
+      }
+      seen.set(a, b);
+      return true;


It's hard for me to tell if this algorithm is correct. Could you add some tests to util_test.ts ?
(You can copy some examples from tools/jasmine_shim_test)

Sure no problem. Just to check first, is this already covered by the tests in jasmine_shim_test.ts? As toEqual is tested in jasmine_shim.ts and it just returns the value from this function. Shall i move the tests from jasmine_shim_test.ts to util_test.ts or copy them?
Thanks for your help, hope you don't mind all the questions!

Yeah, kind of. The jasmine_shim_test.ts is covering the jasmine wrapper around assertEqual (called toEqual)... but we also want to unit test assertEqual by itself. So just duplicating the tests is fine. No, thank you! :)

ry · 2018-03-29T22:45:38Z

src/dl/util.ts

+  }
+}
+
+function equal(c: any, d: any): boolean {


This should be in src/util.ts not src/dl/util.ts

ry · 2018-03-29T22:45:49Z

src/dl/util.ts

@@ -73,6 +73,52 @@ export function assert(expr: boolean, msg: string) {
  }
 }

+export function assertEqual(actual: any, expected: any, msg = null) {


This should be in src/util.ts not src/dl/util.ts

ry · 2018-03-30T03:41:20Z

src/dl/math/ndarray.ts

@@ -222,7 +222,7 @@ export class NDArray<D extends DataType = DataType, R extends Rank = Rank> {

  asScalar(): Scalar<D> {
    this.throwIfDisposed();
-    util.assert(this.size === 1, "The array must have only 1 element.");
+    util.assertEqual(this.size, 1, "The array must have only 1 element.");


Files in src/dl should not be changed.

Works with objects. jasmine_shim.ts and tensor_util.ts use equal/assertEqual from util.ts Relates propelml#423

as assertEqual and equal work with objects (including nested objects). Relates propelml#423

ry

LGTM
Elegantly done - thank you!

dangerdak changed the title ~~[WIPAssert~~ Fixes assertEqual Mar 29, 2018

dangerdak changed the title ~~Fixes assertEqual~~ [WIP] Fixes assertEqual Mar 29, 2018

dangerdak force-pushed the assert branch from 5fe03f5 to 95d363b Compare March 29, 2018 15:42

ry reviewed Mar 29, 2018

View reviewed changes

ry requested a review from piscisaureus March 29, 2018 15:58

dangerdak force-pushed the assert branch from 95d363b to 4b97743 Compare March 29, 2018 17:09

ry reviewed Mar 29, 2018

View reviewed changes

src/dl/util.ts Outdated

}

}

function equal(c: any, d: any): boolean {

Copy link

Contributor

ry Mar 29, 2018

Choose a reason for hiding this comment

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

This should be in src/util.ts not src/dl/util.ts

ry reviewed Mar 29, 2018

View reviewed changes

ry reviewed Mar 30, 2018

View reviewed changes

dangerdak force-pushed the assert branch 2 times, most recently from f144375 to b4b026b Compare March 30, 2018 18:31

ry mentioned this pull request Mar 30, 2018

Use browser like inspector to render output #441

Merged

dangerdak added 2 commits March 31, 2018 08:57

Add assertEqual

6ea98dd

Works with objects. jasmine_shim.ts and tensor_util.ts use equal/assertEqual from util.ts Relates propelml#423

Remove assertObjectsEqual and objectsEqual

cde7ca7

as assertEqual and equal work with objects (including nested objects). Relates propelml#423

dangerdak force-pushed the assert branch from b4b026b to cde7ca7 Compare March 31, 2018 08:10

dangerdak changed the title ~~[WIP] Fixes assertEqual~~ Add assertEqual Mar 31, 2018

ry approved these changes Mar 31, 2018

View reviewed changes

ry merged commit f5e2560 into propelml:master Mar 31, 2018

dangerdak deleted the assert branch March 31, 2018 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assertEqual #457

Add assertEqual #457

dangerdak commented Mar 29, 2018 •

edited

Loading

ry left a comment

ry Mar 29, 2018

ry Mar 29, 2018

ry Mar 29, 2018

dangerdak Mar 29, 2018

ry Mar 29, 2018

ry Mar 29, 2018

dangerdak Mar 29, 2018

ry Mar 29, 2018 •

edited

Loading

dangerdak Mar 31, 2018 •

edited

Loading

ry Mar 31, 2018

ry Mar 29, 2018

ry Mar 29, 2018 •

edited

Loading

dangerdak Mar 30, 2018

ry Mar 30, 2018

ry Mar 29, 2018

ry Mar 29, 2018

ry Mar 30, 2018

ry left a comment

Add assertEqual #457

Add assertEqual #457

Conversation

dangerdak commented Mar 29, 2018 • edited Loading

ry 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

Choose a reason for hiding this comment

ry Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

dangerdak Mar 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Mar 29, 2018 • edited Loading

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

ry left a comment

Choose a reason for hiding this comment

dangerdak commented Mar 29, 2018 •

edited

Loading

ry Mar 29, 2018 •

edited

Loading

dangerdak Mar 31, 2018 •

edited

Loading

ry Mar 29, 2018 •

edited

Loading