Repository: astropy/astropy

Issue:
Refactor utils.diff.report_diff_values
@mcara suggested some refactoring in https://github.com/astropy/astropy/pull/7444#issuecomment-387896174 . While you are at it, also maybe investigate whether https://github.com/astropy/astropy/pull/7444#discussion_r186866768 is possible without breaking regression.

p.s. As long as this gets in for 3.1, it is considered "affects dev" and does not need a change log. This is a direct follow up of #7444.

Discussion:
astropy-bot[bot]: Hi there @mcara :wave: - thanks for the pull request! I'm just a friendly :robot: that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part :smiley:.

I noticed the following issues with this pull request:

* The milestone has not been set (this can only be set by a maintainer)
* Changelog entry not present (or pull request number missing) and neither the **Affects-dev** nor the **no-changelog-entry-needed** label are set

Would it be possible to fix these? Thanks!

*If there are any issues with this message, please report them [here](https://github.com/astropy/astropy-bot/issues).*
mcara: Yeah, I've noticed this mistake (of moving type determination) but was offline to fix it.
pllim: Still fails with the same error. FYI. https://travis-ci.org/astropy/astropy/jobs/379478016
mcara: @pllim @saimn I know... working on it. There are two issues:

1. Why is `repr(of string)` necessary here: https://github.com/astropy/astropy/blob/e80c6c44ea1e3c1b9154f3d34e523db1d5304ec1/astropy/utils/diff.py#L74

2. [most importantly] This and comment above it, seems to be a complete nonsense:

```python
In [2]: a = np.array([[1,2,3,4,7],[5,6,7,8,7],[1,1,1,1,1]])

In [3]: b = np.array([[1,2,3,0,7],[5,6,0,8,7],[1,1,1,1,1]])

In [4]: diff.report_diff_values(a, b, fileobj=sys.stdout)
  at [0, 3]:
line: 'a> 4'
    a> 4
line: 'b> 0'
    b> 0
  at [1, 2]:
line: 'a> 7'
    a> 7
line: 'b> 0'
    b> 0
  ...and at 1 more indices.
Out[4]: False
```

There are ONLY TWO differences between `a` and `b` above!

mcara: @pllim @saimn I know... working on it. There are two issues:

1. Why is `repr(of string)` necessary here: https://github.com/astropy/astropy/blob/e80c6c44ea1e3c1b9154f3d34e523db1d5304ec1/astropy/utils/diff.py#L74

2. [most importantly] This https://github.com/astropy/astropy/blob/e80c6c44ea1e3c1b9154f3d34e523db1d5304ec1/astropy/utils/diff.py#L88 and comment above it, seems to be a complete nonsense:

```python
In [2]: a = np.array([[1,2,3,4,7],[5,6,7,8,7],[1,1,1,1,1]])

In [3]: b = np.array([[1,2,3,0,7],[5,6,0,8,7],[1,1,1,1,1]])

In [4]: diff.report_diff_values(a, b, fileobj=sys.stdout)
  at [0, 3]:
line: 'a> 4'
    a> 4
line: 'b> 0'
    b> 0
  at [1, 2]:
line: 'a> 7'
    a> 7
line: 'b> 0'
    b> 0
  ...and at 1 more indices.
Out[4]: False
```

There are ONLY TWO differences between `a` and `b` above!

pllim: I don't know the history of why it was written that way. This code came from `io.fits`, which in turn came from PyFITS. The "complete nonsense" is my bad; I wrote the comment in an attempt to understand the logic and forgot to remove it. Feel free to improve the comment as you see fit.
mcara: No, the problem is not in the comment - comment just tries to explain the code. I do not understand how there can be 625 different values when a `5x5` array has only 25 values in total!
pllim: > how there can be 625 different values when a 5x5 array has only 25 values in total!

Each 5x5 array has 25 elements in total. And that `reduce` thingy appears to perform `25 * 25` when I ran the logic manually, hence results in `num_diffs = 625` when all elements do not match. But I cannot explain why...
mcara: But then the rest of the logic does not work when, e.g., there are only 2 different indices in `diff_indices` such as in my example. Then code should print 2 indices but it also thinks that it already printed 3 and reports that there is "one more".
pllim: Yeah. I wonder if it shouldn't just be `num_diffs = len(diff_indices[0])`. Even with n-D arrays, that would still work, right?
mcara: I think so.
pllim: I dug a little and it seems to be a `lambda` function prior to #5247 that would still give the same 625 answer. Before that, its history was lost in repo transition. Only @embray knows, if he remembers... :pray: 
```python
num_diffs = reduce(lambda x, y: x * y, (len(d) for d in diff_indices), 1)
```
mcara: In my last commit I restored the logic of using `repr` on strings and on numeric types (but use `str` on _other_ types) from the original implementation. It is not clear to me why this is necessary (i.e., how is `str(1)` different from `repr(1)`?)

I also modified the logic for counting the number of different elements between `ndarrays`. By the way, I do not see how this function can work if arrays have a different number of elements. It should just return `False` but currently, I think, it will crash.

Finally, I have modified the logic when precomputing values in order to minimize the number of `isinstance()` calls, which, in my experience, are slow.
pllim: >  how is str(1) different from repr(1)

Keep in mind that this is heavily used in `fitsdiff` (header, image, table, what-nots) . There is a chance somewhere in the FITS world, `__str__` and `__repr__` give different outputs.
mcara: Well, `repr()` is used only on 1. numeric types (`int`, `float`, `complex`, `np.number`) and 2. sometimes on string types (but only when one argument is `str` and the other one is not). The logic of this is escaping me.
saimn: > how is str(1) different from repr(1)

`repr` has additional quotes around the string. So maybe it is needed when comparing to a non-string value which is also converted to string with `repr`.
mcara: @pllim 

> @mcara , can you please rebase to pick up fix for Sphinx build? Thanks.

Now this is what truly makes this an expert-level issue. 😅 
mcara: @pllim Done rebasing.
mcara: Why is this PR stuck?

pllim: @mcara , as discussed offline, I'll get to this after some other higher priority tasks. Thanks for your patience.
pllim: For the sake of CircleCI, can you please do a rebase (and also `git submodule update` if needed) against `master`? Thanks!

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 978a25f16c..fe20e981c8 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -337,6 +337,9 @@ astropy.units
 astropy.utils
 ^^^^^^^^^^^^^
 
+- Fixed a bug due to which ``report_diff_values()`` was reporting incorrect
+  number of differences when comparing two ``numpy.ndarray``. [#7470]
+
 astropy.visualization
 ^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/astropy/utils/diff.py b/astropy/utils/diff.py
index 07adb92c9c..4a5f87e3bd 100644
--- a/astropy/utils/diff.py
+++ b/astropy/utils/diff.py
@@ -1,9 +1,7 @@
 import difflib
 import functools
-import operator
 import sys
-from functools import reduce
-from itertools import islice
+import numbers
 
 import numpy as np
 
@@ -12,6 +10,7 @@ from .misc import indent
 __all__ = ['fixed_width_indent', 'diff_values', 'report_diff_values',
            'where_not_allclose']
 
+
 # Smaller default shift-width for indent
 fixed_width_indent = functools.partial(indent, width=2)
 
@@ -67,59 +66,72 @@ def report_diff_values(a, b, fileobj=sys.stdout, indent_width=0):
         `True` if no diff, else `False`.
 
     """
-    typea = type(a)
-    typeb = type(b)
-
-    if (isinstance(a, str) and not isinstance(b, str)):
-        a = repr(a).lstrip('u')
-    elif (isinstance(b, str) and not isinstance(a, str)):
-        b = repr(b).lstrip('u')
-
-    if isinstance(a, (int, float, complex, np.number)):
-        a = repr(a)
-
-    if isinstance(b, (int, float, complex, np.number)):
-        b = repr(b)
-
     if isinstance(a, np.ndarray) and isinstance(b, np.ndarray):
-        diff_indices = np.where(a != b)
-        # NOTE: Two 5x5 arrays that are completely different would
-        # report num_diffs of 625 (25 * 25).
-        num_diffs = reduce(operator.mul, map(len, diff_indices), 1)
-        for idx in islice(zip(*diff_indices), 3):
+        if a.shape != b.shape:
             fileobj.write(
-                fixed_width_indent('  at {!r}:\n'.format(list(idx)),
+                fixed_width_indent('  Different array shapes:\n',
                                    indent_width))
-            report_diff_values(a[idx], b[idx], fileobj=fileobj,
+            report_diff_values(str(a.shape), str(b.shape), fileobj=fileobj,
+                               indent_width=indent_width + 1)
+            return False
+
+        diff_indices = np.transpose(np.where(a != b))
+        num_diffs = diff_indices.shape[0]
+
+        for idx in diff_indices[:3]:
+            lidx = idx.tolist()
+            fileobj.write(
+                fixed_width_indent('  at {!r}:\n'.format(lidx), indent_width))
+            report_diff_values(a[tuple(idx)], b[tuple(idx)], fileobj=fileobj,
                                indent_width=indent_width + 1)
 
         if num_diffs > 3:
             fileobj.write(fixed_width_indent(
-                '  ...and at {} more indices.\n'.format(num_diffs - 3),
+                '  ...and at {:d} more indices.\n'.format(num_diffs - 3),
                 indent_width))
+            return False
+
         return num_diffs == 0
 
-    padding = max(len(typea.__name__), len(typeb.__name__)) + 3
+    typea = type(a)
+    typeb = type(b)
+
+    if typea == typeb:
+        lnpad = ' '
+        sign_a = 'a>'
+        sign_b = 'b>'
+        if isinstance(a, numbers.Number):
+            a = repr(a)
+            b = repr(b)
+        else:
+            a = str(a)
+            b = str(b)
+    else:
+        padding = max(len(typea.__name__), len(typeb.__name__)) + 3
+        lnpad = (padding + 1) * ' '
+        sign_a = ('(' + typea.__name__ + ') ').rjust(padding) + 'a>'
+        sign_b = ('(' + typeb.__name__ + ') ').rjust(padding) + 'b>'
+
+        is_a_str = isinstance(a, str)
+        is_b_str = isinstance(b, str)
+        a = (repr(a) if ((is_a_str and not is_b_str) or
+                         (not is_a_str and isinstance(a, numbers.Number)))
+             else str(a))
+        b = (repr(b) if ((is_b_str and not is_a_str) or
+                         (not is_b_str and isinstance(b, numbers.Number)))
+             else str(b))
+
     identical = True
 
-    for line in difflib.ndiff(str(a).splitlines(), str(b).splitlines()):
+    for line in difflib.ndiff(a.splitlines(), b.splitlines()):
         if line[0] == '-':
             identical = False
-            line = 'a>' + line[1:]
-            if typea != typeb:
-                typename = '(' + typea.__name__ + ') '
-                line = typename.rjust(padding) + line
-
+            line = sign_a + line[1:]
         elif line[0] == '+':
             identical = False
-            line = 'b>' + line[1:]
-            if typea != typeb:
-                typename = '(' + typeb.__name__ + ') '
-                line = typename.rjust(padding) + line
+            line = sign_b + line[1:]
         else:
-            line = ' ' + line
-            if typea != typeb:
-                line = ' ' * padding + line
+            line = lnpad + line
         fileobj.write(fixed_width_indent(
             '  {}\n'.format(line.rstrip('\n')), indent_width))
 


Test patch:
diff --git a/astropy/io/fits/tests/test_diff.py b/astropy/io/fits/tests/test_diff.py
index 2f64de1862..68c3566d66 100644
--- a/astropy/io/fits/tests/test_diff.py
+++ b/astropy/io/fits/tests/test_diff.py
@@ -554,7 +554,7 @@ class TestDiff(FitsTestCase):
         assert ('Column A data differs in row 0:\n'
                 '    a> True\n'
                 '    b> False') in report
-        assert ('...and at 13 more indices.\n'
+        assert ('...and at 1 more indices.\n'
                 ' Column D data differs in row 0:') in report
         assert ('13 different table data element(s) found (65.00% different)'
                 in report)
diff --git a/astropy/utils/tests/test_diff.py b/astropy/utils/tests/test_diff.py
index b7138a6ce5..727644a6f8 100644
--- a/astropy/utils/tests/test_diff.py
+++ b/astropy/utils/tests/test_diff.py
@@ -50,10 +50,16 @@ def test_diff_types():
     identical = report_diff_values(a, b, fileobj=f)
     assert not identical
     out = f.getvalue()
-    assert out.lstrip('u') == ("  (float) a> 1.0\n"
-                               "    (str) b> '1.0'\n"
-                               "           ? +   +\n")
+    assert out == ("  (float) a> 1.0\n"
+                   "    (str) b> '1.0'\n"
+                   "           ? +   +\n")
 
+def test_diff_numeric_scalar_types():
+    """ Test comparison of different numeric scalar types. """
+    f = io.StringIO()
+    assert not report_diff_values(1.0, 1, fileobj=f)
+    out = f.getvalue()
+    assert out == '  (float) a> 1.0\n    (int) b> 1\n'
 
 def test_array_comparison():
     """
@@ -74,7 +80,22 @@ def test_array_comparison():
                    '  at [0, 2]:\n'
                    '    a> 2\n'
                    '    b> 3\n'
-                   '  ...and at 78 more indices.\n')
+                   '  ...and at 6 more indices.\n')
+
+
+def test_diff_shaped_array_comparison():
+    """
+    Test diff-ing two differently shaped arrays.
+    """
+    f = io.StringIO()
+    a = np.empty((1, 2, 3))
+    identical = report_diff_values(a, a[0], fileobj=f)
+    assert not identical
+    out = f.getvalue()
+    assert out == ('  Different array shapes:\n'
+                   '    a> (1, 2, 3)\n'
+                   '     ?  ---\n'
+                   '    b> (2, 3)\n')
 
 
 def test_tablediff():