SVGD #1671
SVGD #1671
Conversation
|
This is still WIP. |
pymc3/variational/updates.py
Outdated
| class Update(object): | ||
|
|
||
| def __init__(self): | ||
| self.__dict__.update(locals()) |
There was a problem hiding this comment.
I'm still not satisfied about the current solution.
- Here you don't need init at all
- locals have
selfand create circular reference
pymc3/variational/updates.py
Outdated
|
|
||
| def __init__(self, lr=0.01, *args, **kwargs): | ||
| Update.__init__(self, *args, **kwargs) | ||
| self.__dict__.update(locals()) |
There was a problem hiding this comment.
You update dict twice, moreover passing any additional argument will cause an error, but signature allows it (e.g. SGD(foo=1) will fail, but it is not obvious)
pymc3/variational/updates.py
Outdated
| Update.__init__(self, *args, **kwargs) | ||
| self.__dict__.update(locals()) | ||
|
|
||
| def __call__(self, p, g): |
There was a problem hiding this comment.
Current implementation does not handle the case when list of shared variables is passed, moreover dict is better format of return value. The same comments are about other updates.
pymc3/variational/updates.py
Outdated
| def __init__(self): | ||
| self.__dict__.update(locals()) | ||
|
|
||
| def __call__(self, params, grads): |
There was a problem hiding this comment.
Why not computing grads from objective wrt params(potentially list of params) on the fly? That forces user to do unnecessary work
Update method can be just a helper function overriden below. What call will do is computing gradients and creating updates.
There was a problem hiding this comment.
BTW, signature(interface) changes in inherited classes, it must be the same according to OOP style
There was a problem hiding this comment.
Do you think we should instead use https://github.com/Lasagne/Lasagne/blob/master/lasagne/updates.py?
There was a problem hiding this comment.
@twiecki I think Lasagne has good and reliable implementation. If they don't mind copy-paste that would be great. Otherwise we should probably change api so that it will be the same as Lasagne's api.
|
@ferrine I added updates.py from lasagne. Not sure it works correctly yet. |
|
Great! We can check it with simple advi problem. |
|
@twiecki is there any work left? I'm looking forward merging this PR as it will be useful in many cases |
| @@ -0,0 +1 @@ | |||
| docs/source/notebooks/ | |||
| sumkxy = tt.sum(Kxy, axis=1).dimshuffle(0, 'x') | ||
| dxkxy = tt.add(dxkxy, tt.mul(X, sumkxy)) / (h ** 2) | ||
|
|
||
| return (Kxy, dxkxy) |
|
@ferrine I went ahead and merged to get things moving. I'll address the parentheses thing and add docs in a separate PR. |
No description provided.