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

Prefer tf.ones and tf.zeros over tf.fill #47826

Merged
merged 1 commit into from Mar 18, 2021

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Mar 15, 2021

This PR removes usage of tf.fill in favour of tf.ones or tf.zeros to simplify the code.

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Mar 15, 2021
@google-cla google-cla bot added the cla: yes label Mar 15, 2021
array_ops.fill(
array_ops.shape(y),
constant_op.constant(1, dtype=y.dtype, name="grad_ys_%d" % i)))
array_ops.ones(
array_ops.shape(y), dtype=y.dtype, name="grad_ys_%d" % i))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be further simplified, but would require changes to

def zeros_like(t):
"""Like array_ops.zeros_like, but respects resource handles."""
if t.dtype == dtypes.resource:
return array_ops.zeros(*shape_and_dtype(t))
else:
return array_ops.zeros_like(t)

to accept keyword arguments like name. Let me know if you think that's worth doing.

Copy link
Member

Choose a reason for hiding this comment

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

Does the array_ops.py ones_like work?

def ones_like(tensor, dtype=None, name=None, optimize=True):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's possible since judging from this check, y can be a tf.resource which is not supported by array_ops.ones_like.

Copy link
Member

Choose a reason for hiding this comment

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

None of these (fill, array_ops.zeros, array_ops.zeros_like) support resource-dtype tensors. It looks like it's calling the same "ones" kernel right now, but having it call the standard API would be useful if we ever decide to have a "OnesLike" kernel like we do for zeros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I fully understand your comment. The current code works correctly for resource-dtype tensors since tf.shape can handle resource-dtype tensors, so array_ops.zeros never receives a resource-dtype tensors itself.

Copy link
Member

Choose a reason for hiding this comment

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

Shape works fine, but neither constant_op.constant(1, dtype=dtypes.resource) nor array_ops.ones(shape=..., dtype=dtypes.resource) work. array_ops.ones_like just moves some of the boilerplate to a helper, the functionality is equivalent.

@gbaned gbaned self-assigned this Mar 16, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Mar 16, 2021
@srjoglekar246 srjoglekar246 requested review from allenlavoie and removed request for srjoglekar246 March 16, 2021 17:02
array_ops.fill(
array_ops.shape(y),
constant_op.constant(1, dtype=y.dtype, name="grad_ys_%d" % i)))
array_ops.ones(
array_ops.shape(y), dtype=y.dtype, name="grad_ys_%d" % i))
Copy link
Member

Choose a reason for hiding this comment

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

Does the array_ops.py ones_like work?

def ones_like(tensor, dtype=None, name=None, optimize=True):

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Mar 16, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 16, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 16, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 16, 2021
@lgeiger
Copy link
Contributor Author

lgeiger commented Mar 17, 2021

@allenlavoie I had to update the PR to fix a pylint error on CI, do you mind approving it again?

@gbaned gbaned requested review from allenlavoie and removed request for allenlavoie March 17, 2021 14:48
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 17, 2021
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Mar 17, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Mar 17, 2021
@copybara-service copybara-service bot merged commit da467fc into tensorflow:master Mar 18, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Mar 18, 2021
@lgeiger lgeiger deleted the ones-zeros branch March 18, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants