Skip to content

Increase docstring consistency - #11932

Merged
rmlarsen merged 4 commits into
tensorflow:masterfrom
taehoonlee:improve_doc
Aug 8, 2017
Merged

Increase docstring consistency#11932
rmlarsen merged 4 commits into
tensorflow:masterfrom
taehoonlee:improve_doc

Conversation

@taehoonlee

Copy link
Copy Markdown
Contributor

This PR considers the following six main items:

  • Adjusting indents of markdown code blocks
  • Adding language identifiers on markdown code blocks
  • Replacing Return with Returns (The frequencies over all python codes: 2 vs. 3369)
  • Replacing Arguments with Args
    (Args is a consistent word over all python codes except tensorflow/contrib/keras and tensorflow/python/layers)
  • In tensorflow/python/framework/tensor_shape.py, removing ``` for consistency on the file
  • In tensorflow/python/ops/array_ops.py and math_ops.py, revising python code blocks to make them more concrete
    • I think it is better to replace ==> with # because ==> is not a python keyword.
    • For each operation, two styles (with and without tf.) are mixed. I think it is better to make them as tf.operation not operation.
    • For more concrete examples, I think it is better to write down real tensors not pseudo. (e.g.,# 'x' is [1, 4] is replaced by x = tf.constant([1, 4]).)

@taehoonlee
taehoonlee requested a review from MarkDaoust as a code owner August 1, 2017 07:27
@tensorflow-jenkins

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@MarkDaoust MarkDaoust left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like these changes. Making things more consistent, and making examples more directly runnable is very nice.

Most of my attached comments are just farther consistency improvements.

But please revert the two spots where you changed the code at the same time.

Thanks.

Dimension(None).merge_with(Dimension(n)) == Dimension(n)
Dimension(None).merge_with(Dimension(None)) == Dimension(None)
Dimension(n) .merge_with(Dimension(m)) raises ValueError for n != m

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These blocks don't seem to be pseudo-code.
These are referring to tf.Dimension

A better fix here (and in the rest of this file) might be :

tf.Dimension(n)   .merge_with(tf.Dimension(m))   # raises ValueError for n != m

As you've done in the rest of this PR.

def f3(): return tf.constant(-1)
r = tf.case({tf.less(x, y): f1, tf.greater(x, z): f2},
default=f3, exclusive=True)
```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Someone else just submitted a patch to this section.

You need to resolve the conflict.

Comment thread tensorflow/python/ops/math_ops.py Outdated


def real(input, name=None):
def real(x, name=None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Revert this.

We can't change the name of the argument here.

It will break any code that passes the arg by name.

tf.real(input=1.0+0.2j)

Either way, in a big docs formatting PR like this, no code should change.

Comment thread tensorflow/python/ops/math_ops.py Outdated


def imag(input, name=None):
def imag(x, name=None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

Comment thread tensorflow/python/ops/math_ops.py Outdated
tf.accumulate_n([a, b, a], shape=[2, 2], tensor_dtype=tf.int32)
==> [[7, 4], [6, 14]]
tf.accumulate_n([a, b, a], shape=[2, 2], tensor_dtype=tf.int32) # [[7, 4],
[6, 14]]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing a # on the second line

```
```python
example_names: ["input0", "input1"],
features: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't entirely understand this file, but this block is not valid python as is.

```
```python
x = ... # Tensor of shape [1, 2, 3, 4].
rotate_transpose(x, -1) # result shape: [2, 3, 4, 1]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To make these more concrete this could be:

x = tf.random_normal([1,2,3,4])

Then add a .shape to each rotate_transpose(...)
This way the two == comparison examples work (== on tensors, I think, just checks identity so these currently return False. But rotate_transpose(x, 7).shape == rotate_transpose(x, 3).shape returns True)
(the result comments can also then be reduced to # [3, 4, 2, 1])

Comment thread tensorflow/python/layers/base.py Outdated

```
```python
>>> _unique_layer_name('dense')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the rest of the examples we have no carets, and the result in an inline comment.

You could do the same thing here.

@taehoonlee
taehoonlee force-pushed the improve_doc branch 4 times, most recently from c5fbdb2 to fb80f21 Compare August 3, 2017 14:10
@taehoonlee

Copy link
Copy Markdown
Contributor Author

I agree with the feedback, thank you @MarkDaoust! I reflected all the comments.

These comparison examples need parenthesis to work properly.
Set values to `start`, `limit`, and `delta`
@MarkDaoust

Copy link
Copy Markdown
Member

I am happy with this.

It will also fixes a bunch of inline examples that weren't rendering on the website.

I've re-checked the PR. It is now a pure docs-only change.
All my comments are resolved.
I've added a couple of tiny fixes to some broken examples.

(It could have been a little easier to review the update if you had just added the commit instead of squashing them together.)

Thanks!

@rmlarsen

rmlarsen commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

@tensorflow-jenkins test this please

@rmlarsen
rmlarsen merged commit 2cd0810 into tensorflow:master Aug 8, 2017
@taehoonlee
taehoonlee deleted the improve_doc branch August 9, 2017 00:11
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.

6 participants