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

Improve cis and cispi docs #40859

Merged
merged 14 commits into from
Nov 17, 2021
Merged

Improve cis and cispi docs #40859

merged 14 commits into from
Nov 17, 2021

Conversation

GlenHertz
Copy link
Contributor

I couldn't understand the old docs ("Return \exp(iz)") so I tried to improve them. The iz has no space so I was confused it meant im*z (and as an EE I tend to think j is imaginary). Also the \exp part isn't that great in the REPL as it is a bit confusing what that means. I tried typing \exp in the REPL thinking it may expand to Euler's number but it doesn't work. I guess it is for Latex rendering but that kind of messes up the REPL? I tried to keep with the same format for consistency.

Changes:

  1. Merged cis(real) and cis(complex) into one doc (hopefully I did that right).
  2. More verbose description (adding the benefits of the function).
  3. Explained why it is called cis.
  4. Added more cross-references

base/complex.jl Outdated Show resolved Hide resolved
base/complex.jl Outdated Show resolved Hide resolved
base/complex.jl Outdated Show resolved Hide resolved
@dkarrasch dkarrasch added the docs This change adds or pertains to documentation label May 18, 2021
@GlenHertz
Copy link
Contributor Author

Thanks for the inputs. I've taken another go at it. Please review.

base/complex.jl Outdated

Return ``\\exp(iz)``.
Similar to `exp(im*x)`, this computes ``\\exp(i x)``, but is typically faster.
Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems a bit confusing, why write the same formula twice in a sentence, and what exactly does similar mean, does it mean equal?

My earlier suggestion was to separate two things -- the first line tells you what it returns, that's the basic mathematical fact. The next paragraph tries to explain why this exists instead of exp(im*z), and hence why you may want to use it, which is software detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for stating it twice is once in Julia syntax and once in math/latex/textbook syntax so a reader will understand regardless of what background they are coming from. Also Latex doesn't render in the REPL so having the Julia syntax is helpful.

I agree that "Similar" is a bit confusing as it implies it is not the same. I think it is the same up to 0.5 ULP so I changed it to "Same as"

@ViralBShah ViralBShah added the maths Mathematical functions label May 20, 2021
@GlenHertz
Copy link
Contributor Author

Thanks for the feedback. I tried to address them. Let me know if that is OK.

I fixed the whitespace error with a quick commit and it seemed to have created a race condition. The above whitespace build error doesn't seem to include the latest commit...not sure how to kick it off again.

base/complex.jl Outdated Show resolved Hide resolved
base/complex.jl Outdated Show resolved Hide resolved
@GlenHertz
Copy link
Contributor Author

Is this good to go now?

base/complex.jl Outdated Show resolved Hide resolved
base/complex.jl Outdated Show resolved Hide resolved
@GlenHertz
Copy link
Contributor Author

Took another stab at it. It seems:

  1. mcabbot doesn't like Similar to because what is similar (the result or the method, etc.). I also find it confusing.
  2. stevengj likes Similar to
  3. oscardssmith suggested More efficient method for

So I went with Oscar's suggestion and tried to streamline things a bit (being less repetitive across the doc strings).
It should probably be A more efficient method for but it seems pretty common in technical writing to drop the A.

@ViralBShah ViralBShah closed this Nov 2, 2021
@ViralBShah ViralBShah reopened this Nov 2, 2021
@ViralBShah
Copy link
Member

@GlenHertz Can you rebase this to master?

@ViralBShah ViralBShah merged commit df2abc6 into JuliaLang:master Nov 17, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
* Improve `cis` and `cispi` docs

* Fix whitespace error

* Fix escaping of \\pi

* Add suggestions

* Add sincos refs (more discoverable)

* Typo fix

* Tweaks

* More wordsmithing

* Fix whitespace at EOL

* wordsmithing

* More wordsmithing

* More wordsmithing

* Use larger value of cispi(x) to show accuracy in example
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
* Improve `cis` and `cispi` docs

* Fix whitespace error

* Fix escaping of \\pi

* Add suggestions

* Add sincos refs (more discoverable)

* Typo fix

* Tweaks

* More wordsmithing

* Fix whitespace at EOL

* wordsmithing

* More wordsmithing

* More wordsmithing

* Use larger value of cispi(x) to show accuracy in example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants