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

Fixes UT convergence problem (issue discussed in #1191) #1194

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

MostafaDehghani
Copy link
Contributor

Set the default for "use_memory_as_final_state" flag to False.
Removed the undefined/reduntant flag "use_memory_as_last_state".

NOTE: PR #1192 submitted by @rllin-fathom is basically addressing the same issue, but here I also added the condition of using memory as the final state only when the transition function is an LSTM (to avoid unintentional use of memory for other transition functions).

@googlebot googlebot added the cla: yes PR author has signed CLA label Nov 1, 2018
@MostafaDehghani MostafaDehghani changed the title Fix the issues in #1192 Fixes the issues discussed in #1191 Nov 1, 2018
@MostafaDehghani MostafaDehghani changed the title Fixes the issues discussed in #1191 Fixes the issue discussed in #1191 Nov 1, 2018
@MostafaDehghani MostafaDehghani changed the title Fixes the issue discussed in #1191 Fixes UT convergence problem (issue discussed in #1191) Nov 1, 2018
@crystal0913
Copy link

Why does this parameter use_memory_as_final_state cause convergence problems?

@MostafaDehghani
Copy link
Contributor Author

MostafaDehghani commented Nov 1, 2018

tl;dr
It was just a bug and the convergence of basic UT should be independent of this parameter.

use_memory_as_final_state is used when we use an LSTM as the transition function (use_memory_as_final_state=True means we use the memory instead of the output of LSTM as the output of the final step). I, unfortunately, set it to True in another PR and it messed up the code for the basic universal transformer. Sorry for the inconveniences!

@afrozenator
Copy link
Contributor

@MostafaDehghani - thanks for updating the issues! Merging this now.

@afrozenator afrozenator merged commit cb655f0 into tensorflow:master Nov 1, 2018
tensorflow-copybara pushed a commit that referenced this pull request Nov 1, 2018
PiperOrigin-RevId: 219705888
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.

None yet

4 participants