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

Doc base macros #27949

Merged
merged 10 commits into from
Jul 25, 2018
Merged

Doc base macros #27949

merged 10 commits into from
Jul 25, 2018

Conversation

matbesancon
Copy link
Contributor

A simple PR adding missing documentation to some macros.

Some I couln't find anything to document (or couldn't get the code), like @irrational.

@waldyrious
Copy link
Contributor

waldyrious commented Jul 9, 2018

Some I couln't find anything to document (or couldn't get the code), like @irrational.

A while ago I bookmarked these comments; perhaps they could be useful:

JeffBezanson on 6 March 2014:

the intent of MathConst was to provide a convenient interface to the various functions that exist for computing constants to different precisions, so you don't have to write things like bigfloat_pi(). The intent was not to provide symbolic versions of the constants, such that you might define sin(::MathConst{:pi}) and so on.

Stefan Karpinski on 29 June 2015:

There's a definition for each Irrational of how to compute it as a BigFloat, which can be done to arbitrary precision. Thus an algorithm is provided for each Irrational that can compute it to as many digits as one cares to have.

[...]

[What is] the role of val in @irrational? Is it meant as a quick lookup table in case a float is being requested, to bypass the need to call MPFR? Maybe adding some documentation to the macro or to the lines where the macro is called would be helpful.

It's to avoid bootstrapping issues. At the point where this file is loaded, BigFloat hasn't been loaded yet so we can't call MPFR to compute the Float64 value.

foobar_lv2 on 4 March 2018:

AFAIK Irrational represents a computable number, in the original sense: It is a piece of code that can compute successive approximations (including a-posteriori bounds). Now, arithmetic on computable numbers is nice in math but kinda bad in practice (equality is undecidable!). Hence, sqrt(2) is an eagerly evaluated approximation in julia.

(Note the bold emphasis I added above, which seems relevant to this PR.)

@matbesancon
Copy link
Contributor Author

thanks @waldyrious for the link! We can use the opportunity to document @irrational, even though it's not exported

@matbesancon
Copy link
Contributor Author

cc @JeffBezanson @StefanKarpinski if one of you can check if the description of @irrational makes sense :)

@kshyatt kshyatt added the docs This change adds or pertains to documentation label Jul 10, 2018
@irrational(sym, val, def)

Provides a pre-computed value `val` for an irrational constant
identified by `sum`, along with its definition `def`.
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 explain this as:

Define a new Irrational value, sym, with pre-computed Float64 value val, and
arbitrary-precision definition in terms of BigFloats given be the expression def.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, updated

base/expr.jl Outdated Show resolved Hide resolved
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

I still don’t think this explanation of @pure is correct. The arguments can be mutable but the result cannot depend on the mutable contents. Returning the object_id of an object should be pure, for example. The === function is also pure. Of course, there’s not that many useful functions of mutable objects that are pure but this explanation is still not quite right.

@matbesancon
Copy link
Contributor Author

Ok this will be quicker to find the right doc directly from this thread:


@pure gives the compiler a hint for the definition of a pure function,
helping for type inference.

A pure function can only depend on immutable information.
This also means a @pure function cannot use any global mutable state, including
generic functions. Calls to generic functions depend on method tables which are
mutable global state.
Use with caution, incorrect @pure annotation of a function may introduce
hard to identify bugs. Double check for calls to generic functions.


Would that be ok?

@matbesancon
Copy link
Contributor Author

just re-merged master, some CI tests seemed not to pass anymore

@matbesancon
Copy link
Contributor Author

same thing, circleci didn't pass so I merged the latest master

@matbesancon
Copy link
Contributor Author

All checks passed. @StefanKarpinski given that you gave the last feedback, can you look at the new @pure definition to validate the correctness?

@StefanKarpinski StefanKarpinski merged commit 68744d9 into JuliaLang:master Jul 25, 2018
@StefanKarpinski
Copy link
Member

Thanks for the documentation improvements and sorry for the drawn out review process!

@matbesancon matbesancon deleted the doc-base-macros branch July 25, 2018 20:57
@matbesancon
Copy link
Contributor Author

no trouble, there were more touchy and critical PRs on the stack :)

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

whoops, github is telling me that apparently I had left some comments, but never submitted them 😬

# evaluate p[1] + x * (p[2] + x * (....)), i.e. a polynomial via Horner's rule
"""
@horner(x, p...)
Evaluate p[1] + x * (p[2] + x * (....)), i.e. a polynomial via Horner's rule
Copy link
Member

Choose a reason for hiding this comment

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

Should have one blank line, and be be left-justified

base/int.jl Show resolved Hide resolved

`@pure` gives the compiler a hint for the definition of a pure function,
helping for type inference.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a statement that "This is intended only for internal compiler use, and may change or disappear without warning".

@matbesancon
Copy link
Contributor Author

@vtjnash I'll make another PR for these

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants