Adding logdet #1777

bhargavvader · 2017-02-13T14:03:24Z

This is with regard to #1012.
The code is copy pasted from Theano PR #3959 as suggested in the issue, with the function renamed to logdet.

Should there be tests made for this as well?

twiecki · 2017-02-13T14:07:03Z

Thanks @bhargavvader! Can we just copy the test from the PR?

twiecki · 2017-02-13T14:07:27Z

pymc3/theanof.py

+    matrix M, log(abs(det(M))), on CPU. Avoids det(M) overflow/
+    underflow.
+
+    Note: Once PR #3959 in the Theano repo is merged, this must be removed.


Should add specific link to PR and also mention the original author.

twiecki · 2017-02-13T14:15:49Z

Moreover, would be great if this functionality would already be used in ADVI as part of this PR.

twiecki · 2017-02-13T15:50:10Z

pymc3/tests/test_theanof.py

@@ -18,6 +19,33 @@ def integers_ndim(ndim):
        yield np.ones((2,) * ndim) * i
        i += 1

+class TestLogDet(unittest.TestCase):


Should inherit from SeededTest

bhargavvader · 2017-02-13T16:03:22Z

Addressed code comments.
What changes to ADVI would be needed? Is it in the ELBO part of the code?

twiecki · 2017-02-13T17:40:44Z

https://github.com/pymc-devs/pymc3/blob/master/pymc3/distributions/multivariate.py#L116 here is where we want to use it. Was wrong about ADVI.

bhargavvader · 2017-02-13T18:38:37Z

Cool - so would that be doing result = k * tt.log(2 * np.pi) - logdet(tau) ?
Edit: here, tt.log(1. / det(tau)) = - logdet(tau), right?

Also, the tests are failing, but I think it's related to #1773. The theanof tests seem to be fine.

twiecki · 2017-02-13T19:49:31Z

Edit: here, tt.log(1. / det(tau)) = - logdet(tau), right?

Yes, that looks right.

bhargavvader · 2017-02-15T09:13:43Z

@twiecki , made the changes to multivariate and added logdet to __all__. Could you review it?

twiecki · 2017-02-15T09:15:40Z

pymc3/distributions/multivariate.py

@@ -12,6 +12,8 @@
 from theano.tensor.nlinalg import det, matrix_inverse, trace

 import pymc3 as pm
+
+from pycm3.theanof import logdet


twiecki · 2017-02-15T09:16:25Z

@bhargavvader Make sure that unittests (specifically the distribution ones) still pass locally.

bhargavvader · 2017-02-16T09:26:57Z

I've corrected the typo - had a few doubts about running the tests locally, working on it now.

twiecki · 2017-02-16T11:32:44Z

This is great. I want a second set of eyes on this: @ColCarroll @taku-y @fonnesbeck do you have time to take a look?

bhargavvader · 2017-02-16T12:28:10Z

Update: when I ran nosetests pymc3/tests, there were 9 errors. Both test_distributions.py and test_theanof.py failed with ImportError: No module named nose_parameterized. The other fails were with pymc3.tests.test_stats.TestStats which I reckon is unrelated.

~~Edit: is it possible to run only certain files using nosetest?~~
Figured it out.

ColCarroll · 2017-02-16T12:29:40Z

A recent PR added nose-parametrized. You can pip install it.

…

On Thu, Feb 16, 2017, 7:28 AM Bhargav Srinivasa ***@***.***> wrote: Update: when I ran nosetests pymc3/tests, there were 9 errors. Both test_distributions.py and test_theanof.py failed with ImportError: No module named nose_parameterized. The other fails were with pymc3.tests.test_stats.TestStats which I reckon is unrelated. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1777 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACMHEDLOizyA7oC85bTf1QYIOs7tBKwHks5rdEDcgaJpZM4L_PRt> .

bhargavvader · 2017-02-16T12:51:45Z

All of test_distributions.py, test_distributions_timeseries.py and test_distributions_random.py pass with no errors. (I don't think these run on travis?)

twiecki · 2017-02-16T12:57:08Z

What makes you say they don't run on travis?

Another thing occurred to me: missing GPU support. We're inducing a blocker to run any model with MvNormal on the GPU. I see two solutions:

Add GPU support to logdet (not sure how big of a task that would be)
Add a flag to MvNormal to either use the numerically stable logdet or the manual version.

bhargavvader · 2017-02-16T13:30:17Z

Oh my bad - I noticed that one of the travis checks doesn't include them, but the other one does.

About GPU support, I would be up to the task, but not immediately - I am caught up with some school work and can't devote too much time at the moment. We can add a flag for the moment if that makes sense. Would it be a log, or a warning?

fonnesbeck · 2017-02-16T14:14:51Z

I'm happy with merging this. Perhaps GPU support can be added after the Theano PR is merged?

twiecki · 2017-02-16T14:15:48Z

Here's what I think we should do:

Add a flag to MvNormal: gpu_compat=False with a doc-string explaining the behavior
In __init__ check if gpu_compat is False and theano.config.device == 'gpu' and raise a warning alerting of the incompatibility and notifying about the gpu_compat flag at cost of numerical accuracy.

fonnesbeck · 2017-02-16T14:19:56Z

@twiecki Is that something we think we'd use again? If so, maybe write a decorator that could be added and removed whenever we have (temporary) GPU compatibility issues.

twiecki · 2017-02-16T14:36:15Z

@fonnesbeck Good point. We could put that logic into the Op itself maybe. We might also want to automatically switch in case of GPU and give a warning.

taku-y · 2017-02-16T15:19:52Z

I looked over the code and it looks good to me. It would be nice to merge this PR even if GPU has not been supported yet.

bhargavvader · 2017-02-16T19:52:23Z

Cool - I've added the flag and the warning. I agree with the idea of having a decorator for GPU compatibility issues but I'd rather take it up in a new PR (if we do want it).

twiecki · 2017-02-16T20:11:45Z

pymc3/distributions/multivariate.py

@@ -113,7 +123,7 @@ def logp(self, value):
        delta = value - mu
        k = tau.shape[0]

-        result = k * tt.log(2 * np.pi) + tt.log(1. / det(tau))
+        result = k * tt.log(2 * np.pi) - logdet(tau)


Need to actually check if gpu_compat is true and use the previous statement.

Missed it before - just fixed now.

twiecki · 2017-02-17T08:57:16Z

pymc3/distributions/multivariate.py

@@ -113,7 +124,10 @@ def logp(self, value):
        delta = value - mu
        k = tau.shape[0]

-        result = k * tt.log(2 * np.pi) + tt.log(1. / det(tau))
+        if self.gpu_compat is False:


can just do if self.gpu_compat: ... else: ...

twiecki · 2017-02-17T10:02:47Z

Merging once tests pass.

bhargavvader · 2017-02-17T11:00:42Z

👍

twiecki · 2017-02-17T11:29:29Z

@bhargavvader I just realized that I gave bad advice with placing logdet into theanof, it should really be in math.py. Would it be too much to ask to change that?

ferrine · 2017-02-17T11:40:10Z

pymc3/distributions/multivariate.py

+        if gpu_compat is False:
+            result = k * tt.log(2 * np.pi) - logdet(tau)
+        elif gpu_compat is True:
+            result = k * tt.log(2 * np.pi) + tt.log(1. / det(tau))


result = k * tt.log(2 * np.pi) - tt.log(det(tau)) maybe

or maybe even move the previous part on top and only do result -= logdet(tau)

ferrine · 2017-02-17T11:46:45Z

pymc3/theanof.py

+        o = theano.tensor.scalar(dtype=x.dtype)
+        return Apply(self, [x], [o])
+
+    def perform(self, node, inputs, outputs):


Signature of overriden method changed, should be perform(self, node, inputs, output_storage, params=None) for consistency

That's not crucial thing, so you can take it in account only if you decide moving logdet to math.py

bhargavvader · 2017-02-18T07:59:02Z

I wouldn't mind changing it over to math.py. - should I open a different PR, or will you undo this one? I also wanted to know if there would be interest for a non-GPU compatible decorator like @fonnesbeck was talking about - in that case I can open the 2 PRs in a few days.

twiecki · 2017-02-18T11:10:03Z

Great! Yes, separate PR, also see @ferrine's point. if you have an idea for the decorator sure but I don't think it's high priority.

…

On Feb 18, 2017 8:59 AM, "Bhargav Srinivasa" ***@***.***> wrote: I wouldn't mind changing it over to math.py. - should I open a different PR, or will you undo this one? I also wanted to know if there would be interest for a non-GPU compatible decorator like @fonnesbeck <https://github.com/fonnesbeck> was talking about - in that case I can open the 2 PRs in a few days. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1777 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApJmESiHfB9zPJ-AK1dfytV7amZPX0zks5rdqTHgaJpZM4L_PRt> .

twiecki · 2017-02-19T13:42:11Z

pymc3/distributions/multivariate.py

@@ -113,7 +124,10 @@ def logp(self, value):
        delta = value - mu
        k = tau.shape[0]

-        result = k * tt.log(2 * np.pi) + tt.log(1. / det(tau))
+        if self.gpu_compat:
+            result = k * tt.log(2 * np.pi) + tt.log(1. / det(tau))


the first term can be moved up top, then the if-clause just does -logdet or -tt.log(det(tau)

You can use this code in future, it worked for me

twiecki · 2017-02-25T17:52:40Z

Seems like travis is unhappy about this all of a sudden:

======================================================================
ERROR: test_basic (pymc3.tests.test_math.TestLogDet)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/theano/gof/op.py", line 614, in __call__
    storage_map[ins] = [self._get_test_value(ins)]
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/theano/gof/op.py", line 570, in _get_test_value
    raise AttributeError('%s has no test value %s' % (v, detailed_err_msg))
AttributeError: <TensorType(float64, matrix)> has no test value  
Backtrace when that variable is created:
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/suite.py", line 224, in run
    test(orig)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 45, in __call__
    return self.run(*arg, **kwarg)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 133, in run
    self.runTest(result)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 151, in runTest
    test(result)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/unittest/case.py", line 649, in __call__
    return self.run(*args, **kwds)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/unittest/case.py", line 601, in run
    testMethod()
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 31, in test_basic
    self.validate(test_case_1.astype(theano.config.floatX))
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 15, in validate
    x = theano.tensor.matrix()
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 31, in test_basic
    self.validate(test_case_1.astype(theano.config.floatX))
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 16, in validate
    f = theano.function([x], self.op(x))
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/theano/gof/op.py", line 628, in __call__
    (i, ins, node, detailed_err_msg))
ValueError: Cannot compute test value: input 0 (<TensorType(float64, matrix)>) of Op LogDet(<TensorType(float64, matrix)>) missing default value.  
Backtrace when that variable is created:
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/suite.py", line 224, in run
    test(orig)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 45, in __call__
    return self.run(*arg, **kwarg)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 133, in run
    self.runTest(result)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/site-packages/nose/case.py", line 151, in runTest
    test(result)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/unittest/case.py", line 649, in __call__
    return self.run(*args, **kwds)
  File "/home/travis/miniconda3/envs/testenv/lib/python3.6/unittest/case.py", line 601, in run
    testMethod()
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 31, in test_basic
    self.validate(test_case_1.astype(theano.config.floatX))
  File "/home/travis/build/pymc-devs/pymc3/pymc3/tests/test_math.py", line 15, in validate
    x = theano.tensor.matrix()

Can you take a look @bhargavvader

ferrine · 2017-02-25T23:54:27Z

x = tt.matrix()
x ** 2

fails with the same error

x = tt.matrix()
x.tag.test_value = np.array([[1,1], [2,2]])
x ** 2

not

bhargavvader · 2017-02-27T09:52:55Z

Uh oh. I'll have a look at this.
@ferrine , does it not fail when you set a test value? The error does mention it's missing a default.

bhargavvader · 2017-02-27T16:16:46Z

I'm a little confused about which test_value to use, and what the significance of it is; this link talks about how it's used to help debug, but I don't see how it's relevant here.
When I try running the test after setting a random (2,2) matrix to test_value I get this error ValueError: Cannot compute test value: input 0 (input 0) of Op LogDet(input 0) missing default value.

This is similar to an issue raised by @fonnesbeck , where the solution is just to delete the cache (.theano directory). Do you think it might be a similar issue? Considering how it was fine before but failing now?

bhargavvader · 2017-03-09T20:39:51Z

@twiecki , what's the status of this? Is it still failing?

twiecki · 2017-03-13T22:06:36Z

Seems OK, not sure what caused this.

Adding logdet

95b2046

twiecki reviewed Feb 13, 2017

View reviewed changes

Typos, added tests

9c034c7

twiecki reviewed Feb 13, 2017

View reviewed changes

Seeded test

5b0efc9

Added to __all__, changed distribution

f2729f2

twiecki reviewed Feb 15, 2017

View reviewed changes

Fixed typo

a3602bb

Added GPU warning

3145e97

twiecki reviewed Feb 16, 2017

View reviewed changes

bhargavvader added 2 commits February 17, 2017 12:34

gpu_compat flag

ffa85be

Fixed typo

11b6990

twiecki reviewed Feb 17, 2017

View reviewed changes

Changed if condition

1e0d9b7

twiecki merged commit f4ab79b into pymc-devs:master Feb 17, 2017

twiecki mentioned this pull request Feb 17, 2017

WIP: Implement opvi #1694

Merged

bhargavvader deleted the logdet branch February 17, 2017 11:00

bhargavvader mentioned this pull request Feb 17, 2017

Use logdet() instead of log(det()) #1012

Closed

ferrine reviewed Feb 17, 2017

View reviewed changes

bhargavvader mentioned this pull request Feb 19, 2017

Moving logdet to math #1802

Merged

twiecki reviewed Feb 19, 2017

View reviewed changes

Adding logdet #1777

Adding logdet #1777

Conversation

bhargavvader commented Feb 13, 2017

twiecki commented Feb 13, 2017

Choose a reason for hiding this comment

twiecki commented Feb 13, 2017

Choose a reason for hiding this comment

bhargavvader commented Feb 13, 2017

twiecki commented Feb 13, 2017 • edited Loading

bhargavvader commented Feb 13, 2017 • edited Loading

twiecki commented Feb 13, 2017

bhargavvader commented Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

twiecki commented Feb 15, 2017

bhargavvader commented Feb 16, 2017

twiecki commented Feb 16, 2017

bhargavvader commented Feb 16, 2017 • edited Loading

ColCarroll commented Feb 16, 2017 via email

bhargavvader commented Feb 16, 2017

twiecki commented Feb 16, 2017

bhargavvader commented Feb 16, 2017

fonnesbeck commented Feb 16, 2017

twiecki commented Feb 16, 2017

fonnesbeck commented Feb 16, 2017

twiecki commented Feb 16, 2017

taku-y commented Feb 16, 2017

bhargavvader commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twiecki commented Feb 17, 2017

bhargavvader commented Feb 17, 2017

twiecki commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhargavvader commented Feb 18, 2017

twiecki commented Feb 18, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twiecki commented Feb 25, 2017

ferrine commented Feb 25, 2017 • edited Loading

bhargavvader commented Feb 27, 2017

bhargavvader commented Feb 27, 2017

bhargavvader commented Mar 9, 2017

twiecki commented Mar 13, 2017

twiecki commented Feb 13, 2017 •

edited

Loading

bhargavvader commented Feb 13, 2017 •

edited

Loading

bhargavvader commented Feb 15, 2017 •

edited

Loading

bhargavvader commented Feb 16, 2017 •

edited

Loading

ferrine commented Feb 25, 2017 •

edited

Loading