-
Notifications
You must be signed in to change notification settings - Fork 4
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Add unet and dependencies #11
base: master
Are you sure you want to change the base?
Conversation
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.
Certainly a nice addition. Just docs fall a bit short IMO, sometimes -- in particular if someone is not familiar with tf -- it's a bit guesswork as to what the functions actually do.
__all__ = ('leaky_relu', 'prelu') | ||
|
||
|
||
def leaky_relu(_x, alpha=0.2, name='leaky_relu'): |
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.
Established name?
Reason for the underscore(s) in _x
?
Doc?
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.
The name is established, fixing x
if padding in ('SAME', 'VALID'): | ||
return tf.nn.conv2d(x, W, | ||
strides=strides, padding=padding) | ||
else: |
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.
Kind of strange logic to go into this case for whatever input except 'SAME'
or 'VALID'
comes in.
padding=padding) | ||
|
||
|
||
def conv1dtransp(x, W, stride=1, out_shape=None, padding='SAME'): |
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 this convolving with the flipped kernel? It's a bit hard to imagine what "transposed convolution" is supposed to mean in 1d.
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.
Transpose of the convolution operator, e.g. adjoint.
This is standard nomenclature in the field
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 baffled that tensorflow has no implementation for that. 🤔
strides=strides, padding='VALID') | ||
|
||
|
||
def conv2dtransp(x, W, stride=(1, 1), out_shape=None, padding='SAME'): |
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.
Reason to deviate from the TF naming scheme?
Parameters | ||
---------- | ||
x : `tf.Tensor` with shape ``(B, L, C)`` or ``(B, H, W, C)`` | ||
The input vector/image |
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.
.
else: | ||
raise RuntimeError('unknown activation') | ||
|
||
def apply_conv(x, nout, |
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.
apply_
|
||
return out | ||
|
||
def apply_convtransp(x, nout, |
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.
apply_
|
||
return out | ||
|
||
def apply_maxpool(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.
apply_
if __name__ == '__main__': | ||
from dipp.util.testutils import run_doctests | ||
with tf.Session(): | ||
run_doctests() |
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.
You may want to add extraglobs={'tf': tf}
here as well as to the add_doctest_modules
in pytest_plugins.py
.
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
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.
Commenting here. In general, at least a minimal docstring that says what the function/class does would be great.
Monster commit basically moving most
adler
functionality intodipp