Added atrous_conv1d and 3d. Refactored 2d. #7545

nmiculinic · 2017-02-16T00:33:27Z

This commit makes following changes:

deleted most atrous_conv2d code to reuse existing tf.nn.convolution function
added atrous_conv1d and atrous_conv3d with similar API as atrous_conv2d
Added support for variable rate per dimension, e.g. for atrous_conv2d
rate=2, or rate=[2,1] does different things. Former is equal to
rate=[2,2]. rate_i determines dilation_rate in dimension i.
added strides support with same API as in tf.nn.convolution function

This commit makes more code deletions then additions. However
documentation per function makes it appear large.

Test Plan:

Some simple tests to verify I haven't screwed something up:

A = np.array([1, 2, 3, 4, 5, 6], dtype=np.float32).reshape(1, 6, 1)
print(A)

kernel = np.array([100, 10, 1], dtype=np.float32).reshape(3, 1, 1)

with tf.Session() as sess:
    print(sess.run(tf.nn.atrous_conv1d(A, kernel, padding='SAME', rate=[2])))

B = np.arange(16, dtype=np.float32).reshape(1, 4, 4, 1)
kernel = np.array([1000, 100, 10, 1.0], dtype=np.float32).reshape(2, 2, 1, 1)

with tf.Session() as sess:
    a = sess.run(tf.nn.convolution(B, kernel, padding='SAME', dilation_rate=np.array([2, 2])))
    b = sess.run(tf.nn.atrous_conv2d(B, kernel, rate=2, padding='SAME'))
    print(np.allclose(a, b))
    print(a)
    print(b)

C = np.arange(4**3, dtype=np.float32).reshape(1, 4, 4, 4, 1)
kernel = (10**np.arange(8, 0, -1, dtype=np.float32)).reshape(2, 2, 2, 1, 1)

with tf.Session() as sess:
    a = sess.run(tf.nn.conv3d(C, kernel, strides=[1, 1,1,1,1], padding='SAME'))
    b = sess.run(tf.nn.atrous_conv3d(C, kernel, rate=1, padding='SAME'))
    print(np.allclose(a, b))
    print(a)
    print(b)

Also running atrous_conv2d unit tests to verify backward compatibility.

This commit makes following changes: * deleted most atrous_conv2d code to reuse existing tf.nn.convolution function * added atrous_conv1d and atrous_conv3d with similar API as atrous_conv2d * Added support for variable rate per dimension, e.g. for atrous_conv2d rate=2, or rate=[2,1] does different things. Former is equal to rate=[2,2]. rate_i determines dilation_rate in dimension i. * added strides support with same API as in tf.nn.convolution function This commit makes more code deletions then additions. However documentation per function makes it appear large. Test Plan: Some simple tests to verify I haven't screwed something up: ``` A = np.array([1, 2, 3, 4, 5, 6], dtype=np.float32).reshape(1, 6, 1) print(A) kernel = np.array([100, 10, 1], dtype=np.float32).reshape(3, 1, 1) with tf.Session() as sess: print(sess.run(tf.nn.atrous_conv1d(A, kernel, padding='SAME', rate=[2]))) ``` ``` B = np.arange(16, dtype=np.float32).reshape(1, 4, 4, 1) kernel = np.array([1000, 100, 10, 1.0], dtype=np.float32).reshape(2, 2, 1, 1) with tf.Session() as sess: a = sess.run(tf.nn.convolution(B, kernel, padding='SAME', dilation_rate=np.array([2, 2]))) b = sess.run(tf.nn.atrous_conv2d(B, kernel, rate=2, padding='SAME')) print(np.allclose(a, b)) print(a) print(b) ``` ``` C = np.arange(4**3, dtype=np.float32).reshape(1, 4, 4, 4, 1) kernel = (10**np.arange(8, 0, -1, dtype=np.float32)).reshape(2, 2, 2, 1, 1) with tf.Session() as sess: a = sess.run(tf.nn.conv3d(C, kernel, strides=[1, 1,1,1,1], padding='SAME')) b = sess.run(tf.nn.atrous_conv3d(C, kernel, rate=1, padding='SAME')) print(np.allclose(a, b)) print(a) print(b) ``` Also running atrous_conv2d unit tests to verify backward compatibility.

tensorflow-jenkins · 2017-02-16T00:33:28Z

Can one of the admins verify this patch?

teamdandelion · 2017-03-08T00:03:21Z

@gpapan PTAL

gpapan · 2017-03-08T00:18:01Z

tensorflow/python/ops/nn_ops.py

@@ -903,7 +948,7 @@ def atrous_conv2d(value, filters, rate, padding, name=None):
      the `height` and `width` dimensions. Equivalently, the rate by which we
      upsample the filter values by inserting zeros across the `height` and
      `width` dimensions. In the literature, the same parameter is sometimes
-      called `input stride` or `dilation`.
+      called `input stride` or `dilation`. Altrenatively it could be 3 element sequence denoting dilation rate in each dimension.


s/Altrenatively/Alternatively

gpapan · 2017-03-08T00:18:25Z

tensorflow/python/ops/nn_ops.py

@@ -903,7 +948,7 @@ def atrous_conv2d(value, filters, rate, padding, name=None):
      the `height` and `width` dimensions. Equivalently, the rate by which we
      upsample the filter values by inserting zeros across the `height` and
      `width` dimensions. In the literature, the same parameter is sometimes
-      called `input stride` or `dilation`.
+      called `input stride` or `dilation`. Altrenatively it could be 3 element sequence denoting dilation rate in each dimension.


All lines must have length at most 80 pixels.

gpapan · 2017-03-08T00:21:45Z

tensorflow/python/ops/nn_ops.py

-    # More padding so that rate divides the height and width of the input.
-    pad_bottom_extra = (rate - in_height % rate) % rate
-    pad_right_extra = (rate - in_width % rate) % rate
+  return convolution(input=value, filter=filters, padding=padding, dilation_rate=np.broadcast_to(rate, (2, )), strides=strides, name=name)


If I recall correctly, we had kept the legacy atrous_conv2 as a separate op for backwards compatibility with already exported graphs. Can one of the TF admins comment if this is needed or not?

The underlying BatchToSpace and SpaceToBatch ops are needed for backwards compatibility with exported graphs. Changing the Python wrappers (which change how new graphs are constructed) should not impact that.

gpapan · 2017-03-08T00:22:29Z

tensorflow/python/ops/nn_ops.py

+  the input values every `rate` points.
+  This is equivalent to convolving the input with a set of upsampled filters,
+  produced by inserting `rate - 1` zeros between two consecutive values of the
+  filters along the `height`, `width` and `depth` dimensions, hence the name atrous convolution or convolution with holes (the French word trous means holes in


Line >80 chars long

gpapan · 2017-03-08T00:22:41Z

tensorflow/python/ops/nn_ops.py

+      the `height` and `width` dimensions. Equivalently, the rate by which we
+      upsample the filter values by inserting zeros across the `height` and
+      `width` dimensions. In the literature, the same parameter is sometimes
+      called `input stride` or `dilation`. Altrenatively it could be 3 element sequence denoting dilation rate in each dimension.


Line >80 chars long

gpapan · 2017-03-08T00:22:46Z

tensorflow/python/ops/nn_ops.py


+  """
+  return convolution(input=input, filter=filter, padding=padding, dilation_rate=np.broadcast_to(rate, (3, )), strides=strides, name=name)


Line >80 chars long

gpapan · 2017-03-08T00:22:58Z

tensorflow/python/ops/nn_ops.py

-    # More padding so that rate divides the height and width of the input.
-    pad_bottom_extra = (rate - in_height % rate) % rate
-    pad_right_extra = (rate - in_width % rate) % rate
+  return convolution(input=value, filter=filters, padding=padding, dilation_rate=np.broadcast_to(rate, (2, )), strides=strides, name=name)


Line >80 chars long

gpapan · 2017-03-08T00:23:07Z

tensorflow/python/ops/nn_ops.py

+      padding is other than `'VALID'` or `'SAME'`.
+
+  """
+  return convolution(input=input, filter=filter, padding=padding, dilation_rate=np.broadcast_to(rate, (1, )), strides=strides, name=name)


Line >80 chars long

gpapan

Can you please address some minor comments?

gpapan · 2017-03-08T00:26:31Z

@jbms

jbms · 2017-03-08T01:03:02Z

Removing the duplicate implementation of atrous convolution in the atrous_conv2d function is a desirable change.

As far as adding atrous_conv1d and atrous_conv3d, I don't see the advantage --- better to just use the convolution function directly. atrous_conv2d existed prior to the unified tf.nn.convolution function, and is preserved for backwards compatibility.

nmiculinic · 2017-03-12T15:21:34Z

As far as adding atrous_conv1d and atrous_conv3d, I don't see the advantage --- better to just use the convolution function directly. atrous_conv2d existed prior to the unified tf.nn.convolution function, and is preserved for backwards compatibility.

For both completeness and ease of use. When I was searching for atrous_conv1d I only found atrous_conv2d and assumed only that one is implemented (and I implemented one myself mimicking atrous_conv1d implementation before realizing there's general convolution which handles all cases). Having atrous_conv2d as standalone increase its salience and confuses users (me included).

jbms · 2017-03-13T01:26:50Z

I think this would be better addressed by adding a note to the documentation of atrous_conv2d explaining that it is a legacy interface and that tf.nn.convolution can be used for atrous convolutions of any number of dimensions, rather than adding additional redundant functions.

ahundt · 2017-03-13T04:44:47Z

Some related discussion is going on in #4742, part of which I'll repost here bc it is pertinent.

conv2d_same in tensorflow/models provides both input tensor size and output tensor size:

def conv2d_same(inputs, num_outputs, kernel_size, stride, rate=1, scope=None):
  """[snip...]
  Args:
    inputs: A 4-D tensor of size [batch, height_in, width_in, channels].
  [snip...]
  Returns:
    output: A 4-D tensor of size [batch, height_out, width_out, channels] with
      the convolution output.
  """

I think it would be very productive to add a note in each appropriate function explaining what the output dimensions would be relative to given input dimensions in as they vary by configuration.

nmiculinic · 2017-03-16T16:31:29Z

think this would be better addressed by adding a note to the documentation of atrous_conv2d explaining that it is a legacy interface and that tf.nn.convolution can be used for atrous convolutions of any number of dimensions, rather than adding additional redundant functions.

What is the protocol for function deprecation? Is there some annotation, some specific string in documentation or what?

If I understand correctly, you'd like deleting atrous 1d and 3d, and editing 2d with documentation changes

jbms · 2017-03-16T18:36:51Z

tensorflow/python/ops/nn_ops.py

@@ -903,7 +950,8 @@ def atrous_conv2d(value, filters, rate, padding, name=None):
      the `height` and `width` dimensions. Equivalently, the rate by which we
      upsample the filter values by inserting zeros across the `height` and
      `width` dimensions. In the literature, the same parameter is sometimes
-      called `input stride` or `dilation`.
+      called `input stride` or `dilation`. Alternatively it could be 3 element


This should be "2 element".

jbms · 2017-03-16T19:01:24Z

Yes, remove atrous 1d and 3d.

There is the @deprecated decorator in tensorflow/python/util/deprecation.py. However, since there isn't yet a specific plan/date for removing the atrous_conv2d function, you shouldn't use that in this case.

Instead, you could just add a note to the top of the atrous_conv2d, such as:

This function is a simpler wrapper around the more general @{tf.nn.convolution}, and exists only for backwards compatibility. You can use @{tf.nn.convolution} to perform 1-D, 2-D, or 3-D atrous convolution.

Deleted atrous_conv1d and atrous_conv3d. Added note in documentation for atrous_conv2d that convolution should be used instead.

jbms · 2017-04-10T10:14:02Z

tensorflow/python/ops/nn.py

@@ -31,7 +31,9 @@
 @@depthwise_conv2d
 @@depthwise_conv2d_native
 @@separable_conv2d
+@@atrous_conv1d


These should be removed.

jbms · 2017-04-10T10:14:37Z

tensorflow/python/ops/nn_ops.py

@@ -805,9 +805,14 @@ def op(converted_input, _, converted_padding):  # pylint: disable=missing-docstr
        filter_shape=window_shape)


-def atrous_conv2d(value, filters, rate, padding, name=None):
+def atrous_conv2d(value, filters, rate, padding, strides=None, name=None):


Not clear if it makes sense to add strides, since it is supported by the more generic convolution anyway, but if it is added, it should be documented in the Args section.

drpngx · 2017-04-10T16:52:04Z

Jenkins, test this please.

drpngx · 2017-04-10T16:54:43Z

@jbms @gpapan good to go?

martinwicke · 2017-04-26T15:41:28Z

@jbms @gpapan Ping?

martinwicke · 2017-04-26T15:42:21Z

@nmiculinic there are test failures, can you address those?

jbms · 2017-04-26T23:02:53Z

tensorflow/python/ops/nn_ops.py

-    return value
+  return convolution(input=value, filter=filters, padding=padding,
+                     dilation_rate=np.broadcast_to(rate, (2, )),
+                     strides=strides, name=name)


Test failure is caused by reference to undefined variable strides.

vrv · 2017-05-01T16:27:44Z

@jbms assuming tests pass, is this approved? Thanks!

jbms · 2017-05-01T18:03:57Z

Yes looks good once tests pass.

vrv · 2017-05-01T18:12:59Z

@tensorflow-jenkins test this please

vrv · 2017-05-01T18:13:32Z

I made the fix for the @nmiculinic, let me know if this looks good (hit approve). Thanks!

vrv · 2017-05-02T01:03:37Z

I'm going to assume the build timeout is unrelated since no tests were added. Merigng.

googlebot added the cla: yes label Feb 16, 2017

vrv requested a review from gpapan February 16, 2017 00:34

vrv self-assigned this Feb 16, 2017

vrv added awaiting review Pull request awaiting review stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Feb 16, 2017

vrv assigned gpapan and unassigned vrv Feb 16, 2017

gpapan reviewed Mar 8, 2017

View reviewed changes

teamdandelion added the stat:awaiting response Status - Awaiting response from author label Mar 8, 2017

Fixed > 80 char lines, implemented suggestions

6d182be

jbms mentioned this pull request Mar 13, 2017

Atrous convolution does not preserve tensor shape #4742

Closed

jbms reviewed Mar 16, 2017

View reviewed changes

martinwicke removed awaiting review Pull request awaiting review stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Mar 22, 2017

nmiculinic added 2 commits April 7, 2017 21:57

Implemented suggestions.

2c08ba7

Deleted atrous_conv1d and atrous_conv3d. Added note in documentation for atrous_conv2d that convolution should be used instead.

Restored short summary in atrous_conv2d

bd8fef2

jbms reviewed Apr 10, 2017

View reviewed changes

Fixed cleanup of leftover comments/mentions of atrous_conv1d/3d.

443ae29

martinwicke added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Apr 26, 2017

jbms reviewed Apr 26, 2017

View reviewed changes

Fix undefined strides

2cd78a9

jbms approved these changes May 1, 2017

View reviewed changes

vrv merged commit 880b4ac into tensorflow:master May 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added atrous_conv1d and 3d. Refactored 2d. #7545

Added atrous_conv1d and 3d. Refactored 2d. #7545

nmiculinic commented Feb 16, 2017

tensorflow-jenkins commented Feb 16, 2017

teamdandelion commented Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

jbms Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

gpapan Mar 8, 2017

gpapan left a comment

gpapan commented Mar 8, 2017

jbms commented Mar 8, 2017

nmiculinic commented Mar 12, 2017

jbms commented Mar 13, 2017

ahundt commented Mar 13, 2017

nmiculinic commented Mar 16, 2017

jbms Mar 16, 2017

jbms commented Mar 16, 2017

jbms Apr 10, 2017

jbms Apr 10, 2017

drpngx commented Apr 10, 2017

drpngx commented Apr 10, 2017

martinwicke commented Apr 26, 2017

martinwicke commented Apr 26, 2017

jbms Apr 26, 2017

vrv commented May 1, 2017

jbms commented May 1, 2017

vrv commented May 1, 2017

vrv commented May 1, 2017

vrv commented May 2, 2017


		"""
		return convolution(input=input, filter=filter, padding=padding, dilation_rate=np.broadcast_to(rate, (3, )), strides=strides, name=name)

Added atrous_conv1d and 3d. Refactored 2d. #7545

Added atrous_conv1d and 3d. Refactored 2d. #7545

Conversation

nmiculinic commented Feb 16, 2017

tensorflow-jenkins commented Feb 16, 2017

teamdandelion commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpapan left a comment

Choose a reason for hiding this comment

gpapan commented Mar 8, 2017

jbms commented Mar 8, 2017

nmiculinic commented Mar 12, 2017

jbms commented Mar 13, 2017

ahundt commented Mar 13, 2017

nmiculinic commented Mar 16, 2017

Choose a reason for hiding this comment

jbms commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drpngx commented Apr 10, 2017

drpngx commented Apr 10, 2017

martinwicke commented Apr 26, 2017

martinwicke commented Apr 26, 2017

Choose a reason for hiding this comment

vrv commented May 1, 2017

jbms commented May 1, 2017

vrv commented May 1, 2017

vrv commented May 1, 2017

vrv commented May 2, 2017