Implementation of np.corrcoef #1071

wyjw · 2019-07-28T19:34:17Z

As in #70, an implementation of corrcoef.

There is a small issue during testing in the clipping of a matrix of all nan, which I have described in #1072.

hawkinsp

There appear to be test failures on CPU. Do you think these are all the same bug as #1072 ? As far as I can see that bug only reproduces on GPU, but the NaN disagreement here clearly is happening on CPU since that's the backend our Travis CI builder is using. Is that the case?

Is there a simple test case for bad np.clip semantics that reproduces on CPU too? (You can try CPU by setting the environment variable CUDA_VISIBLE_DEVICES=0.)

Either way, we might need to temporarily avoid generating NaNs in the test to check this in.

hawkinsp · 2019-07-29T13:47:47Z

jax/numpy/lax_numpy.py


+@_wraps(onp.corrcoef)
+def corrcoef(x, y=None, rowvar=True, bias=None, ddof=None):
+  msg = ("jax.numpy.cov not implemented for nontrivial {}. "


Two things:
a) this should probably say "corrcoef" not "cov".
b) can you please move the message into the if condition? No need to allocate a string in the non-error case. Alternatively, you could just omit the error completely, assuming that all we are missing is the corresponding code in jax.numpy.cov.

hawkinsp · 2019-07-29T13:49:53Z

jax/numpy/lax_numpy.py

+  if y is not None: raise NotImplementedError(msg.format('y'))
+
+  c = cov(x, y, rowvar)
+  try:


I'd prefer to check the number of dimensions explicitly rather than relying on an exception from np.diag.

jax/numpy/lax_numpy.py

Corrcoef

googlebot · 2019-07-29T15:17:17Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

wyjw · 2019-07-29T15:19:30Z

I'm sorry I messed up something with my local branch. I'll try to fix it. I'm new to this.

Revert "Corrcoef"

wyjw · 2019-07-29T16:16:42Z

Once again I apologize for the spam of commits. I have changed the code based on the reviews.

As far as my attempt with np.clip and nan go, it still seems to give an error on cpu.

import os
os.environ["CUDA_VISIBLE_DEVICES"]="0"
import jax.numpy as np
a = np.full((3,3,), np.nan)
np.clip(a, -1, 1)

If it helps, the OS I am using is Ubuntu 18.04, Python version 3.6.8 and jax is updated to the latest version.

googlebot · 2019-07-29T16:16:53Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

hawkinsp · 2019-07-29T17:08:01Z

What version of jaxlib do you have? For me, your test case prints:

DeviceArray([[nan, nan, nan],
             [nan, nan, nan],
             [nan, nan, nan]], dtype=float32)

which seems like the right thing (and matches numpy).

hawkinsp · 2019-07-29T17:09:23Z

jax/numpy/lax_numpy.py

+@_wraps(onp.corrcoef)
+def corrcoef(x, y=None, rowvar=True, bias=None, ddof=None):
+  c = cov(x, y, rowvar)
+  if isscalar(c) or (len(c_shape) != 1 and len(c_shape) != 2):


You don't define c_shape, but len(shape(c)) == 0 would seem to cover it.

Note that JAX doesn't distinguish between 0D arrays and scalars.

wyjw · 2019-07-29T17:48:53Z

I have jaxlib 0.1.22. On my machine, the code using cpu still prints a square matrix of -1s.

It seems relevant that the failed Travis case is a mismatch between the two matrices:


self = <lax_numpy_test.LaxBackedNumpyTests testMethod=testCorrCoef_shape=(10, 5)_dtype=<class 'numpy.bool_'>_rowvar=False_ddof=None_bias=False>
x = array([[nan, nan, nan, nan, nan],
       [nan, nan, nan, nan, nan],
       [nan, nan, nan, nan, nan],
       [nan, nan, nan, nan, nan],
       [nan, nan, nan, nan, nan]], dtype=float32)
y = array([[nan, nan, nan, nan, -1.],
       [nan, nan, nan, nan, -1.],
       [nan, nan, nan, nan, -1.],
       [nan, nan, nan, nan, -1.],
       [nan, nan, nan, nan, -1.]], dtype=float32)
check_dtypes = True, atol = 0.01, rtol = 0.01

although it could be something else.

hawkinsp · 2019-07-29T18:11:51Z

Oops! I had an important typo before. This

os.environ["CUDA_VISIBLE_DEVICES"]="0"

should be

os.environ["CUDA_VISIBLE_DEVICES"]=""

So I think that repro is still a GPU bug. It's likely also that setting

XLA_FLAGS="--xla_gpu_enable_fast_min_max=false"

would fix the problem.

I'm still wondering what's happening for the CPU case, though; that sounds like a different issue. (In general NaN semantics aren't heavily tested; for your immediate needs the right fix is to change the test case not to generate NaNs.)

wyjw · 2019-07-29T21:20:03Z

The NaNs were generated by the standard deviation being 0 for some matrices, and calculating the coefficient was done by dividing by the standard deviation. I added an if-case in the test, and it should work now.

Thanks!

hawkinsp

Looks great! Thanks for the PR. One more tiny suggestion.

hawkinsp · 2019-07-30T00:36:30Z

jax/numpy/lax_numpy.py

+@_wraps(onp.corrcoef)
+def corrcoef(x, y=None, rowvar=True, bias=None, ddof=None):
+  c = cov(x, y, rowvar)
+  if isscalar(c) or (len(shape(c)) != 1 and len(shape(c)) != 2):


JAX doesn't distinguish between scalars and 0D arrays. Perhaps there is value in testing for both (e.g., if a numpy classic array is passed here).

Why the two length tests? Why not len(shape(c) == 0?

hawkinsp · 2019-07-30T13:53:55Z

Looks great! Thanks for the PR!

yjwusf and others added 3 commits July 28, 2019 15:17

here

c2f26d7

Implementation of np.corrcoef

07a9047

Update lax_numpy_test.py

fd68636

googlebot added the cla: yes label Jul 28, 2019

hawkinsp reviewed Jul 29, 2019

View reviewed changes

yjwusf and others added 5 commits July 29, 2019 11:05

Merge branch 'master' of https://github.com/google/jax into corrcoef

9740728

made changes to corrcoef

d9b7c5f

here

e2bb9e3

corrcoef

3b6edbb

Merge pull request #1 from wyjw/corrcoef

9856375

Corrcoef

googlebot added cla: no and removed cla: yes labels Jul 29, 2019

wyjw added 6 commits July 29, 2019 11:24

Revert "Corrcoef"

a87627b

Merge pull request #2 from wyjw/revert-1-corrcoef

a106b01

Revert "Corrcoef"

Accidental mistake when reverting.

740ba31

Update pxla.py

4b42bbc

Update pxla.py

4e65c61

Made changes based on review.

a06883d

googlebot added cla: yes and removed cla: no labels Jul 29, 2019

added shape check

5487c78

hawkinsp reviewed Jul 29, 2019

View reviewed changes

shape_c variable taken out

b89e5a7

wyjw added 3 commits July 29, 2019 16:25

Update lax_numpy_test.py

ea8597e

make test more robust

22a6aaf

Update lax_numpy_test.py

5b75a7c

hawkinsp reviewed Jul 30, 2019

View reviewed changes

Update lax_numpy.py

4dcae5d

hawkinsp merged commit 96dd2e6 into jax-ml:master Jul 30, 2019

Implementation of np.corrcoef #1071

Implementation of np.corrcoef #1071

Uh oh!

Conversation

wyjw commented Jul 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkinsp left a comment

Choose a reason for hiding this comment

Uh oh!

hawkinsp Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

hawkinsp Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented Jul 29, 2019

Uh oh!

wyjw commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wyjw commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jul 29, 2019

Uh oh!

hawkinsp commented Jul 29, 2019

Uh oh!

hawkinsp Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

wyjw commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkinsp commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wyjw commented Jul 29, 2019

Uh oh!

hawkinsp left a comment

Choose a reason for hiding this comment

Uh oh!

hawkinsp Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

wyjw Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

hawkinsp commented Jul 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wyjw commented Jul 28, 2019 •

edited

Loading

wyjw commented Jul 29, 2019 •

edited

Loading

wyjw commented Jul 29, 2019 •

edited

Loading

wyjw commented Jul 29, 2019 •

edited

Loading

hawkinsp commented Jul 29, 2019 •

edited

Loading