Skip to content
This repository was archived by the owner on Jul 7, 2023. It is now read-only.

[UT] small bugs in ut_function when one uses data length = 1 #1213

Merged
merged 2 commits into from
Nov 16, 2018

Conversation

cfiken
Copy link
Contributor

@cfiken cfiken commented Nov 12, 2018

Summary

I found and fixed small bugs in UT implements in special case length=1 for universal_transformer_util.py.
Addition to that, I fixed an operation by having some codes out of loop which looks meaningless iteration (this is not main).

Bugs Detail

I found bugs on a special case in universal_transformer_util.py:

  • squeeze operations in ut_function without axis make the dimensions unknown and fail successive operation

The flow for L1136 is below:

  1. the dimensions of state is [batch_size, length, channel] as mentioned in comment at L1110
  2. it reduce to [batch_size, length, 1] in common_layers.dense at L1129
  3. p is [batch_size, length] by p = tf.squeeze(p) at L1136

In this case, the shape of p would be unknown if length = 1 after squeeze operation, and that may makes successive operation fail.

Modification Detail

There are 2 modifications:

  • assign axis at tf.squeeze in ut_function
    • I don't fix L1363 because it has reduce_mean operation before squeeze.
  • have new_state.set_shape(state_shape) out of loop because it looks meaningless 👀

It's ok to revert latter one because it is not a main fix.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no PR author has not signed CLA label Nov 12, 2018
@cfiken
Copy link
Contributor Author

cfiken commented Nov 12, 2018

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has signed CLA and removed cla: no PR author has not signed CLA labels Nov 12, 2018
@afrozenator
Copy link
Contributor

@MostafaDehghani - Can you please help greenlight this? Thanks!

@MostafaDehghani
Copy link
Contributor

Thanks @afrozenator for pinging me. Alwayse enjoy/learn when there is a PR on UT :)
Ah! yes... defining squeeze dimension is needed to not mess up the shape when the length is one. Seems the set_shape doesn't need to be in the loop and taking it out from the loop is correct. Thanks a lot @cfiken for catching these and sending the PR :)

@cfiken
Copy link
Contributor Author

cfiken commented Nov 16, 2018

Thanks @afrozenator for mentioning!
Thanks @MostafaDehghani for checking and accepting my PR 😄
I'm glad I could contribute great project 👍

@afrozenator
Copy link
Contributor

Many thanks @cfiken for fixing and @MostafaDehghani for verifying!

@afrozenator afrozenator merged commit 57db0b7 into tensorflow:master Nov 16, 2018
tensorflow-copybara pushed a commit that referenced this pull request Nov 16, 2018
PiperOrigin-RevId: 221821207
@cfiken cfiken deleted the ut.bugfix.squeeze.dimension branch November 17, 2018 08:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants