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

Airy function cleanup #18050

Merged
merged 2 commits into from
Dec 29, 2016
Merged

Airy function cleanup #18050

merged 2 commits into from
Dec 29, 2016

Conversation

simonbyrne
Copy link
Contributor

This rearranges the different airy functions, deprecating airy, airyx and airyprime in favour of the more specific variants (airyai, airyaiprime, etc.), and clarifies the docs of each remaining function. Fixes #17032.

"""
airy(k,x)
airyai(x::Complex128)
Copy link
Contributor

@tkelman tkelman Aug 16, 2016

Choose a reason for hiding this comment

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

all docstring signature changes need to be made in the RST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks.

@simonbyrne simonbyrne added the maths Mathematical functions label Aug 16, 2016
@@ -1396,47 +1396,53 @@ Mathematical Functions

Compute the polygamma function of order ``m`` of argument ``x`` (the ``(m+1)th`` derivative of the logarithm of ``gamma(x)``\ )

.. function:: airy(k,x)
.. function:: airyai(x)
Copy link
Contributor

@tkelman tkelman Aug 16, 2016

Choose a reason for hiding this comment

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

and they need to match ... the inline docstring has ::Complex128 here, this one doesn't

(probably better without if that's not the only input type that works)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do: I added function airyai end between the docstring and the Complex128 method.

Copy link
Contributor

Choose a reason for hiding this comment

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

the contents don't match

"""
airyai(x::Complex128)
Airy function of the first kind ``\\operatorname{Ai}(x)``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I missed that one. Thanks

@ararslan
Copy link
Member

ararslan commented Aug 16, 2016

This looks great to me! 👍

Once this is merged, any chance you'd want to apply these same changes in JuliaMath/SpecialFunctions.jl? If not I can do so later.

@kshyatt kshyatt added the docs This change adds or pertains to documentation label Aug 16, 2016
@giordano
Copy link
Contributor

Reading the code I understand that all the Airy functions of real arguments are always real, is it correct? So airyaix(-3) != airyaix(complex(-3)). If so, would you mind documenting this?

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Sep 7, 2016

@giordano Thanks, I thought they were, but it turns out not: the Airy functions themselves are real for real values, but the scaled airyaix and airyaiprimex ones are not due to the sqrt(x) term.

The question here is should we return a complex value, or throw a domain error?

@simonbyrne
Copy link
Contributor Author

@stevengj Your thoughts?

@simonbyrne
Copy link
Contributor Author

Note this is also an issue in the existing code:

julia> airyx(-3)
0.35928383932620317

julia> airyx(complex(-3))
0.35928383932620317 - 0.1200641157612309im

@stevengj
Copy link
Member

stevengj commented Sep 7, 2016

Either throw a domain error, or (similar to lgamma) define it to be the analytic complex version for complex x and to be scaled bysqrt(abs(x)) for real x.

@simonbyrne
Copy link
Contributor Author

define it ... to be scaled by sqrt(abs(x)) for real x.

Won't that require changes to the AMOS code?

@giordano
Copy link
Contributor

giordano commented Sep 7, 2016

GSL uses as scaling factor the same as Julia for x>0, and 1 for x < 0 (https://www.gnu.org/software/gsl/manual/html_node/Derivatives-of-Airy-Functions.html). Is this an option for Julia? Of course it would be backward incompatible.

@stevengj
Copy link
Member

stevengj commented Sep 7, 2016

Looking at this more closely, I would suggest throwing a DomainError for negative Real arguments.

For negative arguments, there is no exponential growth or decay that needs to be cancelled, so there is no real need for any scale factor. In that sense, GSL's choice of a scale factor of 1 for x < 0 makes a certain amount of sense.

However, if you switch scale factors to 1 for negative arguments, then any code calling the airyaix function would need to take this into account (if it cares about negative x). So, the caller's code will need an if x < 0 check. But if they are doing an if x < 0 check anyway, they can just call airyai(x) for x < 0 themselves — there is no need for us to provide it.

Furthermore, if we silently switch scale factors for x < 0, then user code that does not check for this case will be incorrect. It is better to throw a DomainError so that they know they need to call the unscaled function manually for the case of negative arguments.

@simonbyrne
Copy link
Contributor Author

I agree with the logic. I've updated the PR and added some more tests for this case.

@simonbyrne simonbyrne added this to the 0.6.0 milestone Nov 18, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 22, 2016

@simonbyrne any update here?

@simonbyrne
Copy link
Contributor Author

I think the only thing holding this back was the decision on what was happening with #4301/#8536, but we should probably still do it. I'll rebase.

@tkelman
Copy link
Contributor

tkelman commented Dec 22, 2016

If #8536 happens soon it should probably incorporate this change anyway.

This rearranges the different airy functions, deprecating `airy`, `airyx` and `airyprime` in favour of the more specific variants (`airyai`, `airyaiprime`, etc.), and clarifies the docs of each remaining function. Fixes #17032.
@simonbyrne
Copy link
Contributor Author

Updated.

@tkelman
Copy link
Contributor

tkelman commented Dec 22, 2016

oops, ignore any travis failures about cmake here, need to restart after https://travis-ci.org/JuliaLang/julia/jobs/186158977 and https://travis-ci.org/JuliaLang/julia/jobs/186158972 have finished and uploaded a new cache

(old logs, for reference sake - not too important though https://gist.github.com/a46d92bdbdfdcd1670fc657f15bee43c and https://gist.github.com/dba158e84bee20504e87f0a77975cd28)

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

(squash) merge when green?

Copy link
Member

@stevengj stevengj left a comment

Choose a reason for hiding this comment

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

LGTM.

@tkelman tkelman merged commit e73c556 into master Dec 29, 2016
@tkelman tkelman deleted the sb/airy branch December 29, 2016 22:57
@giordano
Copy link
Contributor

Just compiled Julia master, but compatibility function doesn't seem to work:

julia> airy(0, 4.0)
ERROR: UndefVarError: afn not defined
Stacktrace:
 [1] airy at ./deprecated.jl:1188 [inlined]
 [2] airy(::Int64, ::Float64) at ./deprecated.jl:1196

@tkelman
Copy link
Contributor

tkelman commented Dec 30, 2016

oops, we should sanity check deprecations in a better way...

might be enough to add a dollar sign inside the QuoteNode calls?

@giordano
Copy link
Contributor

Do you mean

$(QuoteNode($afn))

? I tried that but compilation of Julia fails with

deprecated.jl
error during bootstrap:
LoadError(at "sysimg.jl" line 369: LoadError(at "deprecated.jl" line 1183: ErrorException("error compiling anonymous: syntax: prefix "$" in non-quoted expression")))

giordano added a commit to JuliaPhysics/Measurements.jl that referenced this pull request Dec 30, 2016
In Julia 0.6 Airy functions have been reorganized, see
JuliaLang/julia#18050.

I will keep a method for `airy' until support for Julia 0.5 will be
dropped.
simonbyrne added a commit that referenced this pull request Dec 30, 2016
simonbyrne added a commit that referenced this pull request Dec 30, 2016
simonbyrne added a commit that referenced this pull request Dec 30, 2016
tkelman pushed a commit that referenced this pull request Dec 30, 2016
@nalimilan
Copy link
Member

This broke make docs:

Documenter: setting up build directory.
Documenter: expanding markdown templates.
 !! Undefined binding 'Base.Math.airyaix'. [src/stdlib/math.md]
 !! Undefined binding 'Base.Math.airyaiprimex'. [src/stdlib/math.md]
 !! Undefined binding 'Base.Math.airybix'. [src/stdlib/math.md]
 !! Undefined binding 'Base.Math.airybiprimex'. [src/stdlib/math.md]
Documenter: building cross-references.
 !! No doc found for reference '[`airyaix(z)`](@ref)'. [src/manual/mathematical-operations.md]
 !! No doc found for reference '[`airyaiprimex(z)`](@ref)'. [src/manual/mathematical-operations.md]
 !! No doc found for reference '[`airybix(z)`](@ref)'. [src/manual/mathematical-operations.md]
 !! No doc found for reference '[`airybiprimex(z)`](@ref)'. [src/manual/mathematical-operations.md]
Documenter: running document checks.
 !! Skipped doctesting.
 > checking footnote links.
Documenter: populating indices.
ERROR: LoadError: `makedocs` encountered errors. Terminating build
 in runner(::Type{Documenter.Builder.RenderDocument}, ::Documenter.Documents.Document) at /home/milan/Dev/julia-packaging/julia-master/doc/deps/v0.6/Documenter/src/Builder.jl:202

Maybe we should run it on CI to catch this kind of bug?

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

It did run on CI though https://travis-ci.org/JuliaLang/julia/jobs/187530248#L745 just didn't flag an error?

@nalimilan
Copy link
Member

Sorry, probably a bug on my end then. I guess I used the incorrect Julia executable to build the docs.

@giordano
Copy link
Contributor

The fact that airyaix(x) for negative real x throws a DomainError should be documented? Sorry for being pedantic :-)

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Dec 31, 2016 via email

@giordano
Copy link
Contributor

No, I can do it!

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.

7 participants