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

Minor doc improve #985

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Minor doc improve #985

wants to merge 8 commits into from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Jan 6, 2020

Ready
Doesn't overlap with #853

@aminya aminya force-pushed the docImprove branch 2 times, most recently from 616686e to 0b8a2ff Compare January 6, 2020 12:46
aminya and others added 2 commits January 15, 2020 13:58
Co-Authored-By: Mihir Gadgil <scimas@users.noreply.github.com>
Co-Authored-By: Mike J Innes <mike.j.innes@gmail.com>
@aminya
Copy link
Contributor Author

aminya commented Jan 15, 2020

Could you merge this? Doc improvement PRs should be in bit size, so they get merged fast.
Notice how #853 is waiting to get merged for several months now!

I will create more PRs, as I read more

@aminya aminya changed the title Doc improve Minor doc improve Jan 15, 2020
@janEbert
Copy link
Contributor

Could you merge this? Doc improvement PRs should be in bit size, so they get merged fast.
Notice how #853 is waiting to get merged for several months now!

I will create more PRs, as I read more

You are right. It's still possible to split that PR up, though!
I just thought it would be nice to have a global overhaul.

@aminya
Copy link
Contributor Author

aminya commented Apr 2, 2020

What is holding this back from getting merged? 😞 People will stop contributing if their PRs don't merge.

@@ -20,20 +20,21 @@ julia> d2f(2)
6
```

When a function has many parameters, we can get gradients of each one at the same time:
When a function has many parameters, we can get gradients of each one at the same time by passing each one as an argument:
Copy link
Member

Choose a reason for hiding this comment

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

I would keep only this change and leave the rest as it is since it seems redundant

Copy link
Contributor Author

@aminya aminya Apr 2, 2020

Choose a reason for hiding this comment

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

What changes are redundant? Do you mean the information about the example!?

Copy link
Member

Choose a reason for hiding this comment

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

all changes except the line above, in my opinion. I prefer the previous version

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 am describing the syntax. I don't dee this as redundant.

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

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

Apologies for letting this fall through the cracks. I've added some suggestions to deduplicate the wording and slim down the copy a bit. @CarloLucibello do you mind taking another look?

@@ -20,20 +20,21 @@ julia> d2f(2)
6
```

When a function has many parameters, we can get gradients of each one at the same time:
When a function has many parameters, we can get gradients of each one at the same time by passing each one as an argument:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When a function has many parameters, we can get gradients of each one at the same time by passing each one as an argument:
When a function has many parameters, we can get the gradient with respect to each in a single call:


```jldoctest basics
julia> f(x, y) = sum((x .- y).^2);

```
For this `f`, we can get the gradient with respect to both `x` and `y` in a single call:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For this `f`, we can get the gradient with respect to both `x` and `y` in a single call:

If the "with respect to" is moved up top, this line is no longer necessary.

julia> gradient(f, [2, 1], [2, 0])
([0, 2], [0, -2])
```
where `x = [2, 1]` and `y = [2, 0]`. The above syntax is the same as: `df(x,y) = gradient(f,x,y); df([2,1], [2,0])`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
where `x = [2, 1]` and `y = [2, 0]`. The above syntax is the same as: `df(x,y) = gradient(f,x,y); df([2,1], [2,0])`
The above syntax is the same as: `df(x,y) = gradient(f,x,y); df([2,1], [2,0])`

Copy link
Member

@ToucheSir ToucheSir Jun 14, 2021

Choose a reason for hiding this comment

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

Having a statement like "where ..." is a little cumbersome. Since df(x, y) already names the parameters, there's no need to define them in prose. Alternatively, you could actually declare them in the doctest with x, y = ..., but that might be confusing given f(x, y) has already been defined earlier (users who aren't comfortable with scoping may conflate the two).

@aminya
Copy link
Contributor Author

aminya commented Jun 14, 2021

Feel free to update the branch.

@ToucheSir ToucheSir added this to Needs triage in Triage Jun 19, 2021
@ToucheSir ToucheSir removed this from Needs triage in Triage Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants