Skip to content

Fix test case for dnc util_test.py for TF <=2.5 ,typo and link #218

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from
Open

Fix test case for dnc util_test.py for TF <=2.5 ,typo and link #218

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a39a4f9
Fix test case for dnc utils_test.py for TF <=2.5 ,typos
abhilash1910 Sep 6, 2021
352089c
Revert tf checkpoint and RST x-link change
abhilash1910 Sep 8, 2021
13c4dcc
Merge branch 'deepmind:v2' into v2
abhilash1910 Sep 13, 2021
4c4e6e8
Merge branch 'deepmind:v2' into v2
abhilash1910 Jun 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sonnet/src/nets/dnc/control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@


def get_controller_ctor(controller_name):
"""Returns the constructor for a givn controller name."""
"""Returns the constructor for a given controller name."""
if controller_name == 'LSTM':
return recurrent.LSTM
elif controller_name == 'GRU':
Expand Down Expand Up @@ -100,7 +100,7 @@ def deep_core(control_name,
num_layers: Number of layers.
skip_connections: Boolean that indicates whether to use skip connections.
See documenation for sonnet.DeepRnn in
//learning/deepmind/tensorflow/sonnet/python/modules/basic_rnn.py for more
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for this, I think we can do even better if we use RST to x-link to the actual symbol. Then in our rendered docs (https://sonnet.dev) users can click through to DeepRNN. Concretely, I suggest changing this to:

See documentation for :class:`DeepRNN` for more information.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for this review. Have made the change in the script.

https://github.com/deepmind/sonnet/blob/master/sonnet/python/modules/basic_rnn.py for more
information.
name: module name.

Expand Down
12 changes: 9 additions & 3 deletions sonnet/src/nets/dnc/util_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,15 @@ def testDifferentOutputSizeBreaks(self):
lin_b = linear.Linear(output_size_b, name='lin_b')
input_a = tf.random.uniform([batch_size, input_size])
input_b = tf.random.uniform([batch_size, input_size])
with self.assertRaisesIncompatibleShapesError(
tf.errors.InvalidArgumentError):
util.apply_linear((input_a, input_b), (lin_a, lin_b))
if tf.__version__>="2.5.0":
Copy link
Collaborator

Choose a reason for hiding this comment

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

in general we only maintain compatibility with the latest release of TensorFlow 2, so I would prefer not to merge the change in this file.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @tomhennigan , Thank you for the review. Will revert this to the original version.

with self.assertRaisesIncompatibleShapesError(
tf.errors.InvalidArgumentError):
util.apply_linear((input_a, input_b), (lin_a, lin_b))
else:
with self.assertRaises(
tf.errors.InvalidArgumentError):
util.apply_linear((input_a, input_b), (lin_a, lin_b))


@parameterized.parameters(
{
Expand Down