-
Notifications
You must be signed in to change notification settings - Fork 3
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
Backward convolution #60
base: main
Are you sure you want to change the base?
Conversation
ducoffeM
commented
Mar 13, 2023
- previous method get_affine_components did not scale to large convolution filters
- replace this method by deriving the affine weights with the get_toeplitz method
- get_toeplitz use extract_patches and im2col which is the recommandation of tensorflow for computing the affine versions of convolution
- two versions are implemented in backward_layers/utils_conv: data_format = [channels_first, channels_last]
- associated tests are available in test_backward_utils_conv
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.
Beware:
- The first commit about notebooks belongs to another PR (Cleaning existing notebooks and add a new header #55).
- The new test file mentioned in the description is missing (test_backward_utils_conv.py).
@@ -355,6 +363,43 @@ def get_config(self): | |||
) | |||
return config | |||
|
|||
def get_affine_components(self, x): | |||
"""Conv is a linear operator but its affine component is implicit |
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.
Great description. But if we are to follow PEP-257, "Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description."
So maybe something like
"""Express the implicit affine matrix of the convolution layer.
Conv is a linear operator but its affine component is implicit
we use im2col and extract_patches to express the affine matrix
Note that this matrix is Toeplitz
Args:
x: list of input tensors
Returns:
the affine operators W, b : conv(x)= Wx + b
"""
would be better?
2d05f8b
to
f5ec322
Compare
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 think you can actually squash the last commits into first one (as they are only corrections for the first one). LGTM once the commented test is removed.
tests/test_backward_utils_conv.py
Outdated
K.set_epsilon(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.
OOops, some commented code slipped out ^^.
It should be removed for better readibility.
|
||
Conv is a linear operator but its affine component is implicit | ||
we use im2col and extract_patches to express the affine matrix | ||
Note that this matrix is Toeplitz |
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.
It is okay now. Actually my comment was quite general as other functions in the new code was presenting the same kind of welll detailed but misformatted docstring. Maybe we could cope with all those docstrings in another PR because there is work throughout the library to make all docstrings be compliant with pep 257. Or you can make at least the new docstings respect it as you wish.
When running the tests locally some are failing. Example of failed test:____________________________________________________________________________________________________________ test_toeplitz_from_Decomon[11-3-2-False-channels_first-valid-64] ____________________________________________________________________________________________________________ channels = 11, filter_size = 3, strides = 2, flatten = False, data_format = 'channels_first', padding = 'valid', floatx = 64, helpers = <class 'tests.conftest.Helpers'>
tests/test_backward_utils_conv.py:94: ../../Progs/miniconda3/envs/deco-gpu/lib/python3.9/site-packages/keras/utils/traceback_utils.py:70: in error_handler graph = <tensorflow.python.framework.func_graph.FuncGraph object at 0x7f347584ea30> inputs = [<tf.Tensor 'Placeholder:0' shape=(None, 2, 6, 6) dtype=float64>, <tf.Tensor 'conv2d_862/Conv2D/ReadVariableOp:0' shape=(3, 3, 6, 11) dtype=float64>], control_inputs = []
E ValueError: Exception encountered when calling layer "conv2d_862" (type Conv2D). ../../Progs/miniconda3/envs/deco-gpu/lib/python3.9/site-packages/tensorflow/python/framework/ops.py:1967: ValueError List of failed tests:
|
f5ec322
to
cf1811e
Compare