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

Changes from 1 commit
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
23 changes: 13 additions & 10 deletions pymc3/distributions/dist_math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,18 @@ 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))
Expand All @@ -386,13 +398,4 @@ def grad(self, inputs, grads):
x, = inputs
x_grad, = grads

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')
else:
return [x_grad * self.grad_op(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.