Skip to content

fix bug in maxout function#20455

Merged
case540 merged 3 commits into
tensorflow:r1.9from
lygztq:r1.9
Jul 9, 2018
Merged

fix bug in maxout function#20455
case540 merged 3 commits into
tensorflow:r1.9from
lygztq:r1.9

Conversation

@lygztq

@lygztq lygztq commented Jul 1, 2018

Copy link
Copy Markdown
Contributor

The line "shape[axis] = -1" will make the shape wrong when dealing with batches with arbitrary sizes.
if the shape of input tensor is [None, ... , num_channels, ... ], "shape[axis]=-1" together with "shape+=[num_channels // num_units]" will make the shape become [None, ... , -1, ... , num_channels // num_units]. But when reshape the input tensor, the "None" element in shape will make "-1" become "None", not "num_units".

The line "shape[axis] = -1" will make the shape wrong when dealing with batches with arbitrary sizes.
@yifeif yifeif requested a review from fchollet July 2, 2018 21:19
@yifeif yifeif added the awaiting review Pull request awaiting review label Jul 2, 2018
@gunan gunan added kokoro:run kokoro:force-run Tests on submitted change labels Jul 6, 2018
@kokoro-team kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Jul 6, 2018
@av8ramit

av8ramit commented Jul 6, 2018

Copy link
Copy Markdown

@martinwicke can I merge this?

@martinwicke

Copy link
Copy Markdown
Member

Check with @yifeif but I think yes.

@yifeif

yifeif commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

You are good if it's for non-master branches.

@av8ramit av8ramit added the kokoro:force-run Tests on submitted change label Jul 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 6, 2018
@av8ramit av8ramit added the kokoro:force-run Tests on submitted change label Jul 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 6, 2018
@av8ramit

av8ramit commented Jul 6, 2018

Copy link
Copy Markdown

MacOS flake. Re-running.

@martinwicke martinwicke requested a review from pavithrasv July 9, 2018 16:28
'a multiple of num_units({})'.format(
num_channels, num_units))
shape[axis] = -1
shape[axis] = num_units

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case with None value for an axis?

@lygztq lygztq Jul 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this? I am not sure.

input_tensor = tf.placeholder(tf.float32, [None, 32, 32, 3], name="input_data")
with tf.variable_scope('conv_layer'):
    conv_out = tf.layers.conv2d(input_tensor, 10, (5,5), padding='same', name="conv")
    maxout_out = tf.contrib.layers.maxout(conv_out, 5, axis=3)
#... some test code.
self.assertEqual(maxout_out.get_shape().as_list()[3], 5, "shape in maxout function failed")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically a test case that would have otherwise failed without this change.

@lygztq lygztq Jul 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import numpy as np
import tensorflow as tf

# Suppose this function belongs to some test class
def testMaxoutShape(self):
    data = np.random.randn(64, 32, 32, 3).astype(np.float32)
    input_tensor = tf.placeholder(tf.float32, [None, 32, 32, 3], name="input_data")
    conv_out = tf.layers.conv2d(input_tensor, 10, (5,5), padding='same', name="conv")
    maxout_out = tf.contrib.layers.maxout(conv_out, 5, axis=3)

    with tf.Session() as sess:
        sess.run(tf.global_variables_initializer())
        value = sess.run(maxout_out, feed_dict={input_tensor: data})
    self.assertEqual(value.shape[3], 5, "shape in maxout function failed")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@case540

case540 commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

Going to merge this fix into 1.9 so we can start official release builds. Please follow-up with @pavithrasv comment and add a test case with None please though.

Thanks!

@case540 case540 merged commit 25c197e into tensorflow:r1.9 Jul 9, 2018
@lygztq lygztq deleted the r1.9 branch July 10, 2018 01:36
@lygztq lygztq restored the r1.9 branch July 10, 2018 01:36
@lygztq lygztq deleted the r1.9 branch July 10, 2018 02:06
@lygztq lygztq restored the r1.9 branch July 16, 2018 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review Pull request awaiting review cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants