Repository: numpy/numpy
Pull request: BUG: Fix `np.einsum` errors on Power9 Linux and z/Linux
<!-- Please be sure you are following the instructions in the dev guidelines
http://www.numpy.org/devdocs/dev/development_workflow.html
-->

BUG: fixes #14692 on Power 9 and z/Linux

<!-- We'd appreciate it if your commit message is properly formatted
http://www.numpy.org/devdocs/dev/development_workflow.html#writing-the-commit-message
-->


Problem statement:
BUG: Fix np.einsum errors on Power9 Linux and z/Linux.
An array index fails on Power9 Linux and z/Linux.

PR follows this issue, but not sure that x86 will be happy with the proposed fix

### Reproducing code example:

<!-- A short code example that reproduces the problem/missing feature. It should be
self-contained, i.e., possible to run as-is via 'python myproblem.py' -->

```
(qdev_venv) [linux1@jwoehr1 Qiskit]$ python
Python 3.6.8 (default, Jun 11 2019, 14:52:20) 
[GCC 4.8.5 20150623 (Red Hat 4.8.5-39)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> tensor = np.random.rand(10, 10, 10, 10)
>>> np.einsum('ijij->', tensor)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<__array_function__ internals>", line 6, in einsum
  File "/home/linux1/work/Qiskit/qdev_venv/lib64/python3.6/site-packages/numpy/core/einsumfunc.py", line 1356, in einsum
    return c_einsum(*operands, **kwargs)
ValueError: dimensions in operand 0 for collapsing index 'þ' don't match (-1419377904 != 10)
>>> 
```

<!-- Remove these sections for a feature request -->

### Error message:
```
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<__array_function__ internals>", line 6, in einsum
  File "/home/linux1/work/Qiskit/qdev_venv/lib64/python3.6/site-packages/numpy/core/einsumfunc.py", line 1356, in einsum
    return c_einsum(*operands, **kwargs)
ValueError: dimensions in operand 0 for collapsing index 'þ' don't match (-1419377904 != 10)
```
### Numpy/Python version information:

<!-- Output from 'import sys, numpy; print(numpy.__version__, sys.version)' -->
1.17.2 3.6.8 (default, Jun 11 2019, 14:52:20) 
[GCC 4.8.5 20150623 (Red Hat 4.8.5-39)]

Hints from issue discussion:
Thanks for looking at it. The Poer9 part is the tricky one, CI should tell us if there are issues on platforms with signed char. xref gh-12689, which is probably the identical issue, so if you find a fix we can close that as well :).
@seberg  I'll bet I found the wrong fix :(

There will probably have to be a platform test there, which is not what I coded ...
I am not sure about this, but just out of principle, shouldn't it work to type every single usage of the chars correctly? I am wondering if clang-tidy might already tell me about half the issues, should check.
Yes,  gh-12689 is the same issue.

Now the question is "Did what I coded break `einsum` on x86 platforms?"
When the tests complete, I will be most appreciative of any guidance you can offer on this issue.
I am not a `numpy` guru, I am contributing to IBM Qiskit.
I just installed Qiskit on z/Linux and found this.
I don't really know much about this, pretty certainly not more than discussed in the other issue. But I wonder if we should change the function signature of `parse_operand_subscripts` to use `signed char` and see where it goes from there?
Thank you for promising suggestion. I will work on it tomorrow.
Spent most of today getting later version of CMake to build so that I could build LLVM to get `clang-tidy` :) Building LLVM now ... I guess this exploration will take a few days  ... 
Okay, I've stopped playing with tools and am looking at the code now. I've just learned what Einstein summation is :)
Hot on the track but my connection to z/Linux just went down :( ... more tomorrow.

Commit message: added test case test_einsum_failed_on_p9_and_s390x(self)

Patch to review:
diff --git a/numpy/core/src/multiarray/einsum.c.src b/numpy/core/src/multiarray/einsum.c.src
index e7bbc3d0b284..58af44091921 100644
--- a/numpy/core/src/multiarray/einsum.c.src
+++ b/numpy/core/src/multiarray/einsum.c.src
@@ -1876,7 +1876,7 @@ parse_operand_subscripts(char *subscripts, int length,
      * later where it matters the char is cast to a signed char.
      */
     for (idim = 0; idim < ndim - 1; ++idim) {
-        int label = op_labels[idim];
+        int label = (signed char)op_labels[idim];
         /* If it is a proper label, find any duplicates of it. */
         if (label > 0) {
             /* Search for the next matching label. */
diff --git a/numpy/core/tests/test_einsum.py b/numpy/core/tests/test_einsum.py
index cfeeb8a90d2f..9d6b394071d6 100644
--- a/numpy/core/tests/test_einsum.py
+++ b/numpy/core/tests/test_einsum.py
@@ -700,6 +700,12 @@ def test_einsum_fixed_collapsingbug(self):
         y2 = x[idx[:, None], idx[:, None], idx, idx]
         assert_equal(y1, y2)
 
+    def test_einsum_failed_on_p9_and_s390x(self):
+        # Issues gh-14692 and gh-12689
+        # Bug with signed vs unsigned char errored on power9 and s390x Linux
+        tensor = np.random.random_sample((10, 10, 10, 10))
+        print(np.einsum('ijij->', tensor))
+
     def test_einsum_all_contig_non_contig_output(self):
         # Issue gh-5907, tests that the all contiguous special case
         # actually checks the contiguity of the output


Code review comments:
diff_hunk: @@ -700,6 +700,12 @@ def test_einsum_fixed_collapsingbug(self):
         y2 = x[idx[:, None], idx[:, None], idx, idx]
         assert_equal(y1, y2)
 
+    def test_einsum_failed_on_p9_and_s390x(self):
+        # Issues gh-14692 and gh-12689
+        # Bug with signed vs unsigned char errored on power9 and s390x Linux
+        tensor = np.random.random_sample((10, 10, 10, 10))
+        print(np.einsum('ijij->', tensor))
+
path: numpy/core/tests/test_einsum.py
text: @user1:
Maybe (please check locally before pushing)
```
x = np.einsum('ijij->', tensor)
y = tensor.trace(axis1=0, axis2=2).trace()
assert_equal(x, y)
```

@user2:
With floating point, you can't be sure that `x` and `y` will be *exactly* equal.  Use `assert_allclose` with an appropriately small tolerance, or use an integer array for `tensor` (assuming the test doesn't actually depend on the data type of the array).

@author:
Done; tested on x86_64, power9, and s390x; and pushed.