Skip to content

Raise NotImplementedError for SplineWrapper gradient operation #2211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from May 27, 2017
Merged

Raise NotImplementedError for SplineWrapper gradient operation #2211

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d3d947e
Make spacing more compliant to PEP8
a-rodin May 21, 2017
4d510f4
Raise NotImplementedError from SplineWrapper grad operation
a-rodin May 21, 2017
a85bd3e
Instantiate SplineWrapper recursively to simplify the architecture
a-rodin May 22, 2017
532235a
Create spline derivatives lazily
a-rodin May 22, 2017
8fab095
Move grad_op to a separate property
a-rodin May 27, 2017
b31d6f3
Add tests for SplineWrapper
a-rodin May 27, 2017
5da1ce2
Fix style issues
a-rodin May 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pymc3/distributions/continuous.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from .distribution import Continuous, draw_values, generate_samples, Bound

__all__ = ['Uniform', 'Flat', 'Normal', 'Beta', 'Exponential', 'Laplace',
Expand Down Expand Up @@ -1430,7 +1430,7 @@ def __init__(self, x_points, pdf_points, transform='interval',
Z = interp.integral(x_points[0], x_points[-1])

self.Z = tt.as_tensor_variable(Z)
self.interp_op = DifferentiableSplineWrapper(interp)
self.interp_op = SplineWrapper(interp)
self.x_points = x_points
self.pdf_points = pdf_points / Z
self.cdf_points = interp.antiderivative()(x_points) / Z
Expand Down
27 changes: 15 additions & 12 deletions pymc3/distributions/dist_math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pep8 does not recommend spacing before opening brackets

Copy link
Author

@ghost ghost May 27, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

"""
Creates a theano operation from scipy.interpolate.UnivariateSpline
Expand All @@ -377,22 +378,24 @@ class SplineWrapper (theano.Op):
def __init__(self, spline):
self.spline = spline

@property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

def grad_op(self):
if not hasattr(self, '_grad_op'):
try:
self._grad_op = SplineWrapper(self.spline.derivative())
except ValueError:
self._grad_op = None

if self._grad_op is None:
raise NotImplementedError('Spline of order 0 is not differentiable')
return self._grad_op

def perform(self, node, inputs, output_storage):
x, = inputs
output_storage[0][0] = np.asarray(self.spline(x))

class DifferentiableSplineWrapper (SplineWrapper):
"""
Creates a theano operation with defined gradient from
scipy.interpolate.UnivariateSpline
"""

def __init__(self, spline):
super(DifferentiableSplineWrapper, self).__init__(spline)
self.spline_grad = SplineWrapper(spline.derivative())
self.__props__ += ('spline_grad',)

def grad(self, inputs, grads):
x, = inputs
x_grad, = grads
return [x_grad * self.spline_grad(x)]

return [x_grad * self.grad_op(x)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ghost ghost May 23, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can memorize the call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memorize op creation to be more accurate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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