Raise NotImplementedError for SplineWrapper gradient operation #2211

ghost · 2017-05-21T14:00:45Z

It is necessary to allow guess_scaling fall back to fixed_hessian here instead of just failing.

Fixes #2209.

ferrine · 2017-05-21T16:22:14Z

Better design is to create new spline in the grad without subclassing

twiecki · 2017-05-22T07:34:15Z

@ferrine not sure how you mean.

ferrine · 2017-05-22T08:09:55Z

I do similar thing here https://github.com/Theano/Theano/pull/5963/files#diff-ce7341374a28f4a1b95def7fb580987eR572

ghost · 2017-05-22T13:21:08Z

@ferrine Yes, is a good idea to instantiate objects of a single class recursively instead of having two separate classes.

Fixes #2209

ferrine · 2017-05-22T21:38:15Z

pymc3/distributions/dist_math.py

    itypes = [tt.dscalar]
    otypes = [tt.dscalar]

-    def __init__(self, spline):
+    def __init__(self, spline, n_derivatives):


Why n derivatives? Recursion is ok

n_derivatives is the number of derivatives that a given SplineWrapper object should implement. One wouldn't want to have it larger than necessary, because invocations of UnivariateSpline.derivative() are slow and have O(n) complexity, unbounded recursion doesn't seem to be a good choice here.

ferrine · 2017-05-22T21:40:10Z

pymc3/distributions/dist_math.py

        x, = inputs
        x_grad, = grads
-        return [x_grad * self.spline_grad(x)]
+        return [x_grad * self.grad_op(x)]


You can create new op right here. Pure theano code is expected for grad method

To avoid O(n) calculations on each call of grad method the SciPy spline for the gradient (created by self.spline.derivative()) should be pre-calculated and stored inside of the object. But in this case it is simpler to also store a pre-created Theano operation for the gradient, rather than somehow maintain a list of pre-created SciPy splines and then pass them to the constructor of a Theano operation in the grad method, so that the resulting operation also use a pre-calculated SciPy spline for the n-th order derivative.

You can memorize the call

Memorize op creation to be more accurate

Yes, I thought about it. But I'm concerned that in this case gradient calculation time becomes non-deterministic. For example it might significantly bias tqdm estimation for sampling time.

Why? Functions are compiled after graph is constructed. That will not affect runtime

Hmm, you are right. I added lazy creation of the derivatives.

ferrine · 2017-05-24T07:18:32Z

Lgtm

ferrine · 2017-05-24T07:20:02Z

pymc3/distributions/dist_math.py

+
+        if not hasattr(self, 'grad_op'):
+            try:
+                self.grad_op = SplineWrapper(self.spline.derivative())


What about using property?

I think we don't want to add grad_op to __props__ because it would break __hash__() and __eq__(). For example, this code works now as expected, but wouldn't work if grad_op is added to __props__:

s = InterpolatedUnivariateSpline(...) op1 = SplineWrapper(s) op2 = SplineWrapper(s) op2.grad(...) # calculate gradient assert op1 == op2

One would expect op1 to be equal to op2 because they wrap the same spline, but if spline_op is a property, it won't be the case.

As far as I understand, the purpose of __props__ is to be used for implementation of correct comparisons:

__props__ enables the automatic generation of appropriate __eq__() and __hash__(). Given the method __eq__(), automatically generated from __props__, two ops will be equal if they have the same values for all the properties listed in __props__. Given to the method __hash__() automatically generated from __props__, two ops will be have the same hash if they have the same values for all the properties listed in __props__. __props__ will also generate a suitable __str__() for your op. This requires development version after September 1st, 2014 or version 0.7.

http://deeplearning.net/software/theano/extending/extending_theano.html#op-s-auxiliary-methods

Good point, isn't derivative deterministic? In that case the only prop (spline) is required

Yes, that's why we have only one Theano prop, spline. And grad_op is just a helper property for internal usage, that is needed to avoid unnecessary recalculations of the derivative spline, but not a Theano property.

Under property I meant

@property def grad_op(self):...

But we don't want to expose it to the user, don't we?

I mean what would possible usages be? This class is anyway internal to PyMC3, so I think that we need in it only the functionality that is actually used by PyMC3.

Moreover it would be more pretty

@ferrine Hmm, why not indeed :) Added it in a new commit.

ferrine · 2017-05-27T08:24:35Z

pymc3/distributions/dist_math.py

@@ -378,6 +378,18 @@ class SplineWrapper (theano.Op):
    def __init__(self, spline):
        self.spline = spline

+    @property


ferrine · 2017-05-27T08:27:21Z

Could you please add a simple test that ensures that problem in #2209 is solved?

ferrine

Code looks really great!! I've left some code style nitpicks though

ferrine · 2017-05-27T14:19:49Z

pymc3/distributions/dist_math.py

@@ -365,6 +365,7 @@ def conjugate_solve_triangular(outer, inner):
            grad = tt.triu(s + s.T) - tt.diag(tt.diagonal(s))
        return [tt.switch(ok, grad, floatX(np.nan))]

+
 class SplineWrapper (theano.Op):


pep8 does not recommend spacing before opening brackets

I've fixed all these issues. But just to argue about PEP8... :)

PEP8 forbids spaces after functions or methods, but doesn't say about a whitespace before the base class:

Avoid extraneous whitespace in the following situations:
... (nothing interesting)
Immediately before the open parenthesis that starts the argument list of a function call
... (nothing interesting)

Also, it says that

Method definitions inside a class are surrounded by a single blank line

so a blank line before the first method in a class is legitimate as it surrounds a class method, it is even possible to argue that it is required by PEP8 there.

Actually pep8 linter seems to agree with both ways:

$ cat t.py class Foo (object): def foo(self): print('hi') class Bar(object): def bar(self): print('hi') $ pep8 t.py $ # no errors reported

And as PEP8 is a formalized description of the code style used in Python standard library, we can check what is used in it, and it turns out that both variants are present there:

class ZipInfo (object)
and class LZMAFile(_compression.BaseStream), although class Foo(object) is more frequent than class Foo (object);

class _Outcome(object):\n def __init__(self, result=None): and class _BaseTestCaseContext:\n\n def __init__(self, test_case):, although

class Foo: def __init__(...): ...

is more frequent than

class Foo: def __init__(...): ...

However it was more just to argue, I've already committed the fixes and I see that we need to not just follow PEP8, but also be consistent with other parts of PyMC3, so I agree with your nitpicks and find them to be absolutely reasonable :)

As an off topic, I actually think that it would be nice to have pep8 or flake8 running automatically in Travis and failing builds in case of PEP8 violations. However I briefly investigated this possibility and it seems like it is necessary to either fix some rather large parts of the existing code and/or agree on exceptions from PEP8 that we don't want to follow, like for example maximum line length.

ferrine · 2017-05-27T14:19:54Z

pymc3/distributions/continuous.py

@@ -16,7 +16,7 @@
 from pymc3.theanof import floatX
 from . import transforms

-from .dist_math import bound, logpow, gammaln, betaln, std_cdf, i0, i1, alltrue_elemwise, DifferentiableSplineWrapper
+from .dist_math import bound, logpow, gammaln, betaln, std_cdf, i0, i1, alltrue_elemwise, SplineWrapper


I think this line is too long, we can wrap this import in brackets and do multiline import

ferrine · 2017-05-27T14:19:58Z

pymc3/tests/test_dist_math.py

+
+
+class TestSplineWrapper:
+


blank line is not needed, also it is good to inherit from object

ferrine · 2017-05-27T16:25:43Z

Once tests pass I'll merge. Good job, thanks!

ghost · 2017-05-27T16:26:26Z

Thank you!

ghost changed the title ~~Raise NotImplementedError for SplineWrapper~~ Raise NotImplementedError for SplineWrapper gradient operation May 21, 2017

a-rodin added 3 commits May 22, 2017 16:23

Make spacing more compliant to PEP8

d3d947e

Raise NotImplementedError from SplineWrapper grad operation

4d510f4

Fixes #2209

Instantiate SplineWrapper recursively to simplify the architecture

a85bd3e

ferrine reviewed May 22, 2017

View reviewed changes

Create spline derivatives lazily

532235a

ferrine reviewed May 24, 2017

View reviewed changes

Move grad_op to a separate property

8fab095

ferrine approved these changes May 27, 2017

View reviewed changes

Add tests for SplineWrapper

b31d6f3

ferrine reviewed May 27, 2017

View reviewed changes

Fix style issues

5da1ce2

ferrine merged commit 73694ef into pymc-devs:master May 27, 2017

ghost mentioned this pull request May 27, 2017

NUTS step, when selected explicitly, requires the second derivative #2209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise NotImplementedError for SplineWrapper gradient operation #2211

Raise NotImplementedError for SplineWrapper gradient operation #2211

ghost commented May 21, 2017 •

edited by ghost

Loading

ferrine commented May 21, 2017

twiecki commented May 22, 2017

ferrine commented May 22, 2017

ghost commented May 22, 2017 •

edited by ghost

Loading

ferrine May 22, 2017

ghost May 23, 2017 •

edited by ghost

Loading

ferrine May 22, 2017

ghost May 23, 2017 •

edited by ghost

Loading

ferrine May 23, 2017

ferrine May 23, 2017

ghost May 23, 2017

ferrine May 23, 2017

ghost May 24, 2017

ferrine commented May 24, 2017

ferrine May 24, 2017

ghost May 24, 2017 •

edited by ghost

Loading

ferrine May 24, 2017

ghost May 24, 2017 •

edited by ghost

Loading

ferrine May 24, 2017

ghost May 24, 2017

ghost May 24, 2017 •

edited by ghost

Loading

ferrine May 24, 2017

ferrine May 24, 2017

ghost May 27, 2017 •

edited by ghost

Loading

ferrine May 27, 2017

ferrine commented May 27, 2017

ferrine left a comment

ferrine May 27, 2017

ghost May 27, 2017 •

edited by ghost

Loading

ferrine May 27, 2017

ferrine May 27, 2017

ferrine commented May 27, 2017

ghost commented May 27, 2017

Raise NotImplementedError for SplineWrapper gradient operation #2211

Raise NotImplementedError for SplineWrapper gradient operation #2211

Conversation

ghost commented May 21, 2017 • edited by ghost Loading

ferrine commented May 21, 2017

twiecki commented May 22, 2017

ferrine commented May 22, 2017

ghost commented May 22, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

ghost May 23, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 23, 2017 • edited by ghost Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrine commented May 24, 2017

Choose a reason for hiding this comment

ghost May 24, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 24, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 24, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 27, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrine commented May 27, 2017

ferrine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 27, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrine commented May 27, 2017

ghost commented May 27, 2017

ghost commented May 21, 2017 •

edited by ghost

Loading

ghost commented May 22, 2017 •

edited by ghost

Loading

ghost May 23, 2017 •

edited by ghost

Loading

ghost May 23, 2017 •

edited by ghost

Loading

ghost May 24, 2017 •

edited by ghost

Loading

ghost May 24, 2017 •

edited by ghost

Loading

ghost May 24, 2017 •

edited by ghost

Loading

ghost May 27, 2017 •

edited by ghost

Loading

ghost May 27, 2017 •

edited by ghost

Loading