- Notifications
You must be signed in to change notification settings - Fork107
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add support for tensorflow together with ODL#972
Conversation
Phew.. My review here against your review on#861? :-P |
It's on its way :) But you wait until I rebase from master, this is actually quite a small PR. |
Is this ready for review or should I wait? |
Still havn't merged from master, but you can easily get started if you want to. |
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.
I guess since this code is battle-tested there's not very much to say about the implementation itself. I just wonder if temporaries would make sense to speed up things, but that might quickly become infeasible. Dunno.
In general I find the wrapping code hard to digest, not because of complexity (it's really not that elaborate) but more because of all the inline function definitions that take up quite some space. If there's a way to make this a bit less nested, I'd appreciate it.
Anyway, docs could use some cleanup. Other than that it looks good (nice detective work on the ninja-style workarounds).
| What is vectorization? | ||
| ====================== |
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.
Has this stuff moved here from some other file?
In any case, can you convert it to the "one sentence - one line" RST style?
| ============ | ||
| Python functions are in most cases used as input to a discretization process. For example, we may | ||
| want to discretize a two-dimensional Gaussian function: |
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.
Gaussian function ::
| @@ -0,0 +1,38 @@ | |||
| """Example of how to convert an ODL operator to a tensorflow layer.""" | |||
| import tensorflow as tf | |||
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.
Are we sure thatodl.contrib.tensorflow is never confused withtensorflow (e.g.odl.contrib.tensorflow.tensorflow)?
Iftensorflow wasn't already so long I'd suggest using abindings suffix.. Maybe a shorter package name liketf_bindings will cut it?
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.
I'm quite sure that is a nonproblem. With that said I'll change this whole thing to a submodule and split it, that allows better long-term extensions.
| # Create tensorflow layer from odl operator | ||
| odl_op_layer = odl.contrib.tensorflow.as_tensorflow_layer( | ||
| odl_op, 'MatrixOperator') |
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.
indentation
| print(odl_op(x)) | ||
| # Evaluate the adjoint of the derivative, called gradient in tensorflow | ||
| print(tf.gradients(y_tf, [x_tf], z_tf)[0].eval().ravel()) |
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.
Perhaps use keyword arguments to make the code more self-explanatory. Now it's a bit of guesswork as to what variable stands for what.
odl/contrib/tensorflow.py Outdated
| self.name = name | ||
| def _lincomb(self, a, x1, b, x2, out): | ||
| with tf.name_scope('{}_lincomb'.format(self.name)): |
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.
Will this clash if you use the same name twice?
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.
No, it becomes_lincomb_2 etc
odl/contrib/tensorflow.py Outdated
| return isinstance(other, TensorflowSpace) and other.shape == self.shape | ||
| def __repr__(self): | ||
| return 'TensorflowSpace({})'.format(self.shape) |
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.
Consider subclassers
odl/contrib/tensorflow.py Outdated
| self._data = value | ||
| def __repr__(self): | ||
| return '{}.element({})'.format(self.space, self.data) |
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.
{!r}
odl/contrib/tensorflow.py Outdated
| """Wrap ODL operator so that it acts on TensorflowSpace elements.""" | ||
| def __init__(self, domain, range, func, adjoint=None, linear=False): |
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.
Better use the same name for parameter and stored attribute.
odl/contrib/tensorflow.py Outdated
| return tensorflow_layer | ||
| class TensorflowSpace(LinearSpace): |
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.
A bit more docs for the fellas here and below wouldn't hurt.
And, to repeat one of the review comments, a top-level README is quite important for this stuff. |
Thanks for the review, I'll try to get this done and merged rather soon but I'm waiting for the FOM pull (and more free time for me) since I'll move some stuff into there. |
Fixed the comments, added some tests etc. IMO this can go in now. |
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.
Some final comments, merge when done.
Clearly a very cool addition!
odl/contrib/tensorflow/README.md Outdated
| ## Example usage | ||
| The [examples](examples) folder contains example on how to use the above functionality. |
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.
examples
odl/contrib/tensorflow/README.md Outdated
| * [tensorflow_layer_matrix.py](examples/tensorflow_layer_matrix.py) shows how an ODL `MatrixOperator` can be converted to a tensorflow layer. | ||
| * [tensorflow_layer_productspace.py](examples/tensorflow_layer_productspace.py) shows how an ODL operator acting on `ProductSpace`s can be converted to a tensorflow layer. | ||
| * [tensorflow_layer_ray_transform.py](examples/tensorflow_layer_ray_transform.py) shows how a `RayTransform` can be converted to a tensorflow layer. | ||
| * [tensorflow_operator_matrix.py](examples/tensorflow_operator_matrix.py) shows how `tf.matmul` can be used as a ODL operator. |
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.
an ODL
| step = learning_rate * np.sqrt(1 - beta2) / (1 - beta1) | ||
| x.lincomb(1, x, -step, m / (np.sqrt(v) + eps)) |
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.
Did you check this with the paper? Regarding your earlier answer to my review comment, yes, I read the paper, and I couldn't match the expressions there with the implementation here.
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.
I'll review this once more then I guess!
odl/tomo/backends/astra_cuda.py Outdated
| @@ -62,6 +63,10 @@ def __init__(self, geometry, reco_space, proj_space): | |||
| self.create_ids() | |||
| # Create a mutually exclusive lock so that two callers cant use the | |||
| # same shared resource at the same time. | |||
| self.mutex = Lock() | |||
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.
bump (prefer this a bit more hidden,_mutex)
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.
Agree, will fix that.
| @@ -802,6 +803,42 @@ def _apply_padding(lhs_arr, rhs_arr, offset, pad_mode, direction): | |||
| working_slc[axis] = intersec_slc[axis] | |||
| def zscore(arr): | |||
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.
Is that your naming or is it a standard notion?
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.
https://se.mathworks.com/help/stats/zscore.html
Is the motivation.
There is also:
https://docs.scipy.org/doc/scipy-0.14.0/reference/generated/scipy.stats.mstats.zscore.html
But it does not handle constants "well".
Merge after CI |
TODO: summary