Skip to content
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

Update layers.py #13829

Closed
wants to merge 2 commits into from
Closed

Update layers.py #13829

wants to merge 2 commits into from

Conversation

shreyneil
Copy link
Contributor

I have added a check on beta with reference to the following issue raised.
#11673

Please verify and get back.
Thank you.

I will add update the unit test cases to verify this patch.

I have added a check on beta with reference to the following issue raised.
tensorflow#11673

Please verify and get back.
Thank you.
@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@vrv vrv self-assigned this Oct 19, 2017
@vrv
Copy link

vrv commented Oct 20, 2017

@tensorflow-jenkins test this please

@vrv vrv added the kokoro:force-run Tests on submitted change label Oct 20, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 20, 2017
@vrv
Copy link

vrv commented Oct 20, 2017

Please add the test as requested in the previous version of the CL, otherwise this looks great, thanks!

@vrv vrv added the stat:awaiting response Status - Awaiting response from author label Oct 20, 2017
shreyneil added a commit to shreyneil/tensorflow that referenced this pull request Oct 20, 2017
The following PR contains the required unit test case regarding issue tensorflow#11673 
and it also has a fix in another pull request tensorflow#13829
@shreyneil
Copy link
Contributor Author

#13864 contains the updated unit test case.

@shreyneil shreyneil closed this Oct 20, 2017
@shreyneil shreyneil reopened this Oct 20, 2017
@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?




class zero_debias_moving_mean(test.TestCase):
Copy link

Choose a reason for hiding this comment

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

In this case, you can just add the test to the existing BatchNormTest suite, no need to add a new ones.

In general though, you can check the style of other class names in this file; one should use CamelCase test names here if you were adding a new class.


class zero_debias_moving_mean(test.TestCase):

def Error_in_tf.contrib.layers.batch_norm_when(self):
Copy link

Choose a reason for hiding this comment

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

test names should start with 'test':

def testBatchNormCenterFalse

class zero_debias_moving_mean(test.TestCase):

def Error_in_tf.contrib.layers.batch_norm_when(self):
import tensorflow as tf
Copy link

Choose a reason for hiding this comment

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

In internal tests, you have to import the modules individually, you cannot import tensorflow here. So you should use something like:

a = array_ops.placeholder(dtypes.float32, ...)

Take a look at the existing batch norm tests and use that as a guide for how to write the test where center=False

@vrv
Copy link

vrv commented Oct 25, 2017

It looks like you may be having trouble updating this PR to include all the changes -- perhaps it makes sense to resend a new fresh PR with all of the requested changes (layers.py and layers_test.py change in the same PR). Thanks!

@vrv vrv closed this Oct 25, 2017
shreyneil added a commit to shreyneil/tensorflow that referenced this pull request Nov 22, 2017
The Changes have been made as suggested in my previous pull request.
tensorflow#13829

The unit test case has been updated along with the update in layers.py with reference to the issue tensorflow#11673. 
Please verify and get back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes stat:awaiting response Status - Awaiting response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants