Assert macros: minimize spacings between values in context window #2263

mark-poscablo · 2018-08-06T19:44:06Z

Following up on #2257, the spacing between consecutive values in the context window (when values differ at some point) has been reduced in this change. It was previously hardcoded as 10 space chars - now, the output field width is the maximum between the string lengths of the dim0 position, the output value, and the reference value, so any unnecessary spaces are removed.

This also fixes a bug when u8 (unsigned char) is used. Previously, the context displays the character representation of the value. It's fixed now so it displays the number representation instead.

In addtion, this also changes the implementation of ravelIdx() from an explicit multiply-and-accumulate to using std::inner_product()

Here's a sample error message:

VALUE DIFFERS at (100):
Viewing slice (95:105), dims are (200)
        95     96     97     98     99     [100]    101    102    103    104    105    
 out: { 2.2345 2.2345 2.2345 2.2345 2.2345 [1]      2.2345 2.2345 2.2345 2.2345 2.2345 }
gold: { 2.2345 2.2345 2.2345 2.2345 2.2345 [2.2345] 2.2345 2.2345 2.2345 2.2345 2.2345 }
  Actual diff: 1.2344999313354492
Expected diff: 0.0010000000474974513

Using "normal" equality for u8 (unsigned char) type:

VALUE DIFFERS at (125, 82):
Viewing slice (120:130, 82), dims are (200, 100)
        120 121 122 123 124 [125] 126 127 128 129 130 
 out: { 173 50  245 245 75  [2]   10  10  76  239 188 }
gold: { 173 50  245 245 75  [138] 10  10  76  239 188 }

And using c32 (complex float):

VALUE DIFFERS at (3, 0, 1, 2):
Viewing slice (0:8, 0, 1, 2), dims are (200, 5, 4, 3)
        0                    1                     2                      [3]                    4                    5                    6                    7                    8                    
 out: { (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334) [(2, 0)]               (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422) }
gold: { (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334) [(0.024198, 0.999783)] (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422) }

umar456

Looks good. This might not be the most efficient way to do this but it works. It would be nice if the values were right-aligned instead of left.

umar456 · 2018-08-06T23:52:10Z

test/testHelpers.hpp

+//  as numbers
+
+template<typename T>
+std::string operator+(const T& val) {


This is not necessary. You can remove this function. You only need to overload for the af::cfloat and af::cdouble.

👍 That makes sense. The whole point is just to avoid the + being interpreted as an addition for af complex types, and the stream << operator is already overloaded for those types, so simply returning the same value is enough.

umar456 · 2018-08-06T23:52:57Z

test/testHelpers.hpp

+}
+
+template<>
+std::string operator+(const af::cfloat& val) {


This function should just return the same value. It shouldn't convert to a string. That is done by the operator<<

umar456 · 2018-08-06T23:53:06Z

test/testHelpers.hpp

+
+template<>
+std::string operator+(const af::cdouble& val) {
+    std::ostringstream os;


Same as above.

umar456 · 2018-08-06T23:55:26Z

test/testHelpers.hpp

+        std::ostringstream tmpOs;
+
+        uint dim0 = dim0Start + i;
+        if (dim0 == coords[0])


Use braces even if there is only one statement in the body of the if condition.

umar456 · 2018-08-06T23:55:50Z

test/testHelpers.hpp

+    std::vector<int> valsWidths;
+    std::vector<std::string> ctxDim0;
+    std::vector<std::string> ctxOutVals;
+    std::vector<std::string> ctxGoldVals;


Reserve these vectors if you know the size ahead of time.

👍 Yeah that's faster than having vectors resize from time to time as it grows

umar456 · 2018-08-07T00:06:06Z

test/testHelpers.hpp

+
+        int maxWidth = std::max<int>(dim0Len, outLen);
+        maxWidth = std::max<int>(maxWidth, goldLen);
+        valsWidths.push_back(maxWidth);


Is it necessary to store the lengths of each of the values? I would have thought max would be enough.

I suppose the max of all the string lengths would be enough, but I thought if there's one value that's abnormally long (compared to the rest of the values), or if the indices are long compared to the values, then there would be a lot of unnecessary spaces as well. Thus I decided to just use the maximum per element between the index, the out value, and the reference value. What do you think?

It's not a big deal. This code shouldn't be running all the time anyway.

umar456 · 2018-08-07T00:07:55Z

test/testHelpers.hpp


-    // Display dim0 positions
+    // Display dim0 positions, output values, and reference values
+


Extra line.

umar456 · 2018-08-07T00:10:02Z

test/testHelpers.hpp

-        } else
-            os << std::setw(valsWidth) << std::left << i << " ";
+    for (uint i = 0; i < (dim0End - dim0Start); ++i) {
+        os << std::setw(valsWidths[i]) << std::left << ctxDim0[i]


Would you need the extra space at the end if you added one to the setw value?

👍 Turns out that I don't!

umar456 · 2018-08-07T00:12:15Z

test/testHelpers.hpp

+
+    // Get dim0 positions and out/reference values for the context window
+    // Also get the max string length between the position and out/ref values
+    //  per item so that it can be used later as the field width for


Extra space at the beginning of the comment line.

mark-poscablo · 2018-08-07T02:53:43Z

Here are the sample error messages after the changes:

VALUE DIFFERS at (100):
Viewing slice (95:105), dims are (200)
         95     96     97     98     99     [100]    101    102    103    104    105    
 out: {  2.2345 2.2345 2.2345 2.2345 2.2345      [1] 2.2345 2.2345 2.2345 2.2345 2.2345}
gold: {  2.2345 2.2345 2.2345 2.2345 2.2345 [2.2345] 2.2345 2.2345 2.2345 2.2345 2.2345}
  Actual diff: 1.2344999313354492
Expected diff: 0.0010000000474974513

VALUE DIFFERS at (125, 82):
Viewing slice (120:130, 82), dims are (200, 100)
         120 121 122 123 124 [125] 126 127 128 129 130 
 out: {  173  50 245 245  75   [2]  10  10  76 239 188}
gold: {  173  50 245 245  75 [138]  10  10  76 239 188}

VALUE DIFFERS at (3, 0, 1, 2):
Viewing slice (0:8, 0, 1, 2), dims are (200, 5, 4, 3)
         0                    1                     2                      [3]                    4                    5                    6                    7                    8                    
 out: {  (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334)               [(2, 0)] (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422)}
gold: {  (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334) [(0.024198, 0.999783)] (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422)}

mark-poscablo · 2018-08-07T03:00:32Z

I had to fix the uneven border spaces near the braces haha

umar456 · 2018-08-07T03:01:14Z

Maybe the indices should be right/center aligned as well. the complex example looks weird.

mark-poscablo · 2018-08-07T03:19:53Z

Haha true. Here's the outputs now:

VALUE DIFFERS at (100):
Viewing slice (95:105), dims are (200)
            95     96     97     98     99    [100]    101    102    103    104    105
 out: { 2.2345 2.2345 2.2345 2.2345 2.2345      [1] 2.2345 2.2345 2.2345 2.2345 2.2345 }
gold: { 2.2345 2.2345 2.2345 2.2345 2.2345 [2.2345] 2.2345 2.2345 2.2345 2.2345 2.2345 }
  Actual diff: 1.2344999313354492
Expected diff: 0.0010000000474974513

VALUE DIFFERS at (125, 82):
Viewing slice (120:130, 82), dims are (200, 100)
        120 121 122 123 124 [125] 126 127 128 129 130
 out: { 173  50 245 245  75   [2]  10  10  76 239 188 }
gold: { 173  50 245 245  75 [138]  10  10  76 239 188 }

VALUE DIFFERS at (3, 0, 1, 2):
Viewing slice (0:8, 0, 1, 2), dims are (200, 5, 4, 3)
                           0                     1                      2                    [3]                    4                    5                    6                    7                    8
 out: { (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334)               [(2, 0)] (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422) }
gold: { (0.478302, 0.220143) (0.0281664, 0.524339) (0.0122882, 0.0084334) [(0.024198, 0.999783)] (0.181692, 0.274965) (0.372441, 0.669381) (0.022832, 0.717884) (0.373681, 0.060741) (0.824519, 0.816422) }

mark-poscablo requested a review from umar456 August 6, 2018 19:44

mark-poscablo added the test label Aug 6, 2018

mark-poscablo force-pushed the fix-context-spacings branch from 349156a to 35c36c2 Compare August 6, 2018 20:51

umar456 requested changes Aug 7, 2018

View reviewed changes

mark-poscablo force-pushed the fix-context-spacings branch from 35c36c2 to 97a9ee2 Compare August 7, 2018 02:46

mark-poscablo force-pushed the fix-context-spacings branch from 97a9ee2 to 088a83f Compare August 7, 2018 02:59

Assert macros: minimize spacings between values in context window

df774f3

mark-poscablo force-pushed the fix-context-spacings branch from 088a83f to df774f3 Compare August 7, 2018 03:15

umar456 approved these changes Aug 7, 2018

View reviewed changes

9prady9 merged commit 5d02259 into arrayfire:master Aug 7, 2018

mark-poscablo deleted the fix-context-spacings branch October 29, 2018 17:51

mlloreda added this to the v3.6.2 milestone Nov 14, 2018


		// Display dim0 positions
		// Display dim0 positions, output values, and reference values

Assert macros: minimize spacings between values in context window #2263

Assert macros: minimize spacings between values in context window #2263

Uh oh!

Conversation

mark-poscablo commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umar456 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-poscablo commented Aug 7, 2018

Uh oh!

mark-poscablo commented Aug 7, 2018

Uh oh!

umar456 commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mark-poscablo commented Aug 7, 2018

Uh oh!

Uh oh!

mark-poscablo commented Aug 6, 2018 •

edited

Loading

umar456 commented Aug 7, 2018 •

edited

Loading