Skip to content

Fix GRUBlockCell parameter naming inconsistency - #13153

Merged
caisq merged 3 commits into
tensorflow:masterfrom
yongtang:13137-GRUBlockCell-num_units
Sep 23, 2017
Merged

caisq merged 3 commits into
tensorflow:masterfrom
yongtang:13137-GRUBlockCell-num_units

Conversation

@yongtang

Copy link
Copy Markdown
Member

This fix tries to fix the issue in #13137 where parameter cell_size is used instead of num_units. This is inconsistent with other RNN cells.

This fix adds support of num_units while at the same time maintains backward compatiblility for cell_size.

This fix fixes #13137.

Signed-off-by: Yong Tang [email protected]

This fix tries to fix the issue in 13137 where
parameter `cell_size` is used instead of `num_units`.
This is inconsistent with other RNN cells.

This fix adds support of `num_units` while at the same
time maintains backward compatiblility for `cell_size`.

This fix fixes 13137.

Signed-off-by: Yong Tang <[email protected]>
This commit adds `@deprecated_args` for 'cell_size' in `GRUBlockCell`

Signed-off-by: Yong Tang <[email protected]>
@tensorflow-jenkins

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@mention-bot

Copy link
Copy Markdown

@yongtang, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tensorflower-gardener, @rohan100jain and @mrry to be potential reviewers.

@drpngx

drpngx commented Sep 22, 2017

Copy link
Copy Markdown
Contributor

@ebrevdo WDYT?

@drpngx drpngx added the awaiting review Pull request awaiting review label Sep 22, 2017
@drpngx
drpngx requested a review from ebrevdo September 22, 2017 16:22
"""Initialize the Block GRU cell.

Args:
cell_size: int, GRU cell size.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

must keep the old arg in the order it shows up in the signature; just say it's deprecated and means the same thing as num_units.

add a Raises: section with entry "ValueError: if both cell_size and num_units are not None; or both are None."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @ebrevdo. The docstring has been updated.

num_units: int, The number of units in the GRU cell
"""
self._cell_size = cell_size
if cell_size is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if (cell_size is None) == (num_units is None):
raise ValueError("Exactly one of num_units or cell_size must be provided.")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ebrevdo Thanks. Indeed that is much cleaner. Updated.

Signed-off-by: Yong Tang <[email protected]>
@yongtang

Copy link
Copy Markdown
Member Author

@ebrevdo Thanks for the review. The PR has been updated. Please take a look.

@drpngx drpngx added awaiting testing (then merge) and removed awaiting review Pull request awaiting review labels Sep 23, 2017
@drpngx

drpngx commented Sep 23, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@drpngx

drpngx commented Sep 23, 2017

Copy link
Copy Markdown
Contributor

Timeout or reboot of the executor, probably.

Jenkins, test this please.

@caisq
caisq merged commit e0501bc into tensorflow:master Sep 23, 2017
@yongtang
yongtang deleted the 13137-GRUBlockCell-num_units branch September 23, 2017 14:23
caisq added a commit to caisq/tensorflow that referenced this pull request Sep 26, 2017
END_PUBLIC

---
Commit 1e1b3d9 authored by Pete Warden<[email protected]>
Committed by gunan<[email protected]>:
Changed output directory for Pi CI build to fix permissions problem with nightlies (tensorflow#13257)

* Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change for Pi that's no longer needed

* Fixed Pi Zero OpenBLAS build problems and tidied up directories used

* More robust checks in Pi build script

* Changed output directory for Pi CI build to fix permissions problem

---
Commit fe3a2e6 authored by Yan Facai (???)<[email protected]>
Committed by drpngx<[email protected]>:
check invalid string type for dest_nodes in extract_sub_graph (tensorflow#13057)

* BUG: check str type

* TST: add unit test

* CLN: remove list check

* CLN: use warning

* CLN: 2 indent

* CLN: raise TypeError if not list

* CLN: check string only

---
Commit 225ab76 authored by Jean Wanka<[email protected]>
Committed by Jean Wanka<[email protected]>:
Fix polynomial decay with cycle for global step=0

For polynomial decay with cycle=True the learning rate at
step 0 becomes NaN, because in the process of calculating it we
devide by 0. This change should fix it, by setting the multiplier
for the decay steps to one for global_step=0.

---
Commit 286f570 authored by Bjarke Hammersholt Roune<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Make Service::TransferToClient not attempt to manipulate the literal when the transfer failed, preventing a crash and allowing the caller to see the reason for the failed transfer.

PiperOrigin-RevId: 169770126

---
Commit e0501bc authored by Yong Tang<[email protected]>
Committed by Shanqing Cai<[email protected]>:
Fix GRUBlockCell parameter naming inconsistency (tensorflow#13153)

* Fix GRUBlockCell parameter naming inconsistency

This fix tries to fix the issue in 13137 where
parameter `cell_size` is used instead of `num_units`.
This is inconsistent with other RNN cells.

This fix adds support of `num_units` while at the same
time maintains backward compatiblility for `cell_size`.

This fix fixes 13137.

Signed-off-by: Yong Tang <[email protected]>

* Add `@deprecated_args` for 'cell_size' in `GRUBlockCell`

This commit adds `@deprecated_args` for 'cell_size' in `GRUBlockCell`

Signed-off-by: Yong Tang <[email protected]>

* Address review comment

Signed-off-by: Yong Tang <[email protected]>

---
Commit 02a2eba authored by Pete Warden<[email protected]>
Committed by gunan<[email protected]>:
Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change that's no longer needed (tensorflow#13251)

* Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change for Pi that's no longer needed

* Fixed Pi Zero OpenBLAS build problems and tidied up directories used

* More robust checks in Pi build script

---
Commit 8ef7222 authored by Sanjoy Das<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Remove a redundant setName.

The EmitComputation should have emitted a function with the right name, so use a
CHECK instead.

PiperOrigin-RevId: 169764856

---
Commit 1b94147 authored by Neal Wu<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Fix broken GitHub links in tensorflow and tensorflow_models resulting from The Great Models Move (a.k.a. the research subfolder)

PiperOrigin-RevId: 169763373

---
Commit b1ada5f authored by Justine Tunney<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Fix TensorBoard python -m invoke in docs

PiperOrigin-RevId: 169758752

---
Commit 2957cd8 authored by Mustafa Ispir<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Local run option of estimator training.

PiperOrigin-RevId: 169756384

---
Commit 1dc2fe7 authored by Gunhan Gulsoy<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
BEGIN_PUBLIC
Automated g4 rollback of changelist 166264198

PiperOrigin-RevId: 169998124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RNN cells parameter naming inconsistency

7 participants