-
Notifications
You must be signed in to change notification settings - Fork 267
Conversation
// Get single-dimensional index. | ||
int idx = PyInt_AsLong(index); | ||
if (idx == -1 && PyErr_Occurred()) return nullptr; | ||
if (idx < 0 || idx >= format->dim(0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: you can support the Python convention here:
if (idx < 0) idx += format->dim(0);
python/myelin/gradient.py
Outdated
def neg_grad(op, g): | ||
x = op.inputs[0] | ||
y = op.outputs[0] | ||
g.add(x, g.expr.mul(g.d(y), g.expr.neg(g.v(x)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this just be:
g.add(x, g.expr.neg(g.d(y))) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This bug has already been fixed in the C++ version.
python/myelin/gradient.py
Outdated
def reciprocal_grad(op, g): | ||
x = op.inputs[0] | ||
y = op.outputs[0] | ||
g.add(x, g.expr.mul(g.d(y), g.expr.neg(g.expr.rcp(g.expr.square(g.v(x)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g.expr.neg(g.expr.square(g.v(y))) might be more efficient than g.expr.neg(g.expr.rcp(g.expr.square(g.v(x))))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. This is my autograd experiment in python which was included in this PR by mistake.
However, the optimization is still good since you save a division, so I have changed this in the C++ version. Thanks for spotting this.
doc/guide/myelin.md
Outdated
|
||
 | ||
|
||
The graph only shows the input and output variables (gren and blue), and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: gren -> green
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/guide/myelin.md
Outdated
|
||
The weights in W and b can be initialized from NumPy arrays or any other | ||
objects that support the | ||
[Python bufffer protocol](https://docs.python.org/2/c-api/buffer.html): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: bufffer -> buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review.
python/myelin/gradient.py
Outdated
def reciprocal_grad(op, g): | ||
x = op.inputs[0] | ||
y = op.outputs[0] | ||
g.add(x, g.expr.mul(g.d(y), g.expr.neg(g.expr.rcp(g.expr.square(g.v(x)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. This is my autograd experiment in python which was included in this PR by mistake.
However, the optimization is still good since you save a division, so I have changed this in the C++ version. Thanks for spotting this.
python/myelin/gradient.py
Outdated
def neg_grad(op, g): | ||
x = op.inputs[0] | ||
y = op.outputs[0] | ||
g.add(x, g.expr.mul(g.d(y), g.expr.neg(g.v(x)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This bug has already been fixed in the C++ version.
doc/guide/myelin.md
Outdated
|
||
 | ||
|
||
The graph only shows the input and output variables (gren and blue), and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/guide/myelin.md
Outdated
|
||
The weights in W and b can be initialized from NumPy arrays or any other | ||
objects that support the | ||
[Python bufffer protocol](https://docs.python.org/2/c-api/buffer.html): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
You can now run Myelin computations from Python using pysling. I have updated flow.py to support flow version 5 format. You can now compile these flows to a network to run computations. I have updated the documentation with examples of running Myelin in Python.
The Myelin tensors support the Python buffer interface, so these can be shared with other Python modules with buffer support, like numpy.