[GitHub] [incubator-mxnet] D-Roberts edited a comment on pull request #18757: [Numpy] Add qr backward for wide inputs with nrows < ncols

[GitBox <gi...@apache.org>]

D-Roberts edited a comment on pull request #18757:
URL: https://github.com/apache/incubator-mxnet/pull/18757#issuecomment-661264530


   Hi @leezu .
   By 're-verify tests' I really meant that I ran the tests >3 times with MXNET_TEST_COUNT=10000 .
   
   I also made the following changes to the tests (as compared to the previous PR):
   
   1. I removed the expensive analytical and numerical Jacobian calculations that were included in the initial PR. These calculations were initially included to demonstrate that the analytical method that I was implementing was working correctly and passing the central differences numerical checks. At the time of the initial PR 3 months ago, MXNet was the only framework implementing this qr backward method for wide inputs. However, in the meanwhile (over the past 3 months), TensorFlow had this method implemented, so the method is already verified via central differences. Notice in the example given in the description that the results from MXNet implementation and Tf implementation are identical. 
   
   2. I included a larger matrix in the test input for a test case. I removed a couple of redundant test cases to reduce test run time. I also relaxed atol and rtol for dtype float32 to make sure there are no flaky test runs at any time due to numerical rounding errors.
   
   The error that led to the "stale PR" failing CI was due to a calculation in the numerical Jacobian for central differences check. The code broke after updates to Numpy and MXNet Numpy when the source array is float32 and the the dtype used in view is float64.
   ```
   "jacobian_num[row, :] = diff.asnumpy().ravel().view(dtype)
   [2020-07-17T17:41:26.700Z] E           ValueError: When changing to a larger dtype, its size must be a divisor of the total size in bytes of the last axis of the array.
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org