-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Airy function cleanup #18050
Conversation
""" | ||
airy(k,x) | ||
airyai(x::Complex128) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks.
@@ -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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Lines 43 to 46 in 54ec215
""" | |
airyai(x::Complex128) | |
Airy function of the first kind ``\\operatorname{Ai}(x)``. |
There was a problem hiding this comment.
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
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. |
Reading the code I understand that all the Airy functions of real arguments are always real, is it correct? So |
@giordano Thanks, I thought they were, but it turns out not: the Airy functions themselves are real for real values, but the scaled The question here is should we return a complex value, or throw a domain error? |
@stevengj Your thoughts? |
Note this is also an issue in the existing code: julia> airyx(-3)
0.35928383932620317
julia> airyx(complex(-3))
0.35928383932620317 - 0.1200641157612309im |
Either throw a domain error, or (similar to |
Won't that require changes to the AMOS code? |
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. |
Looking at this more closely, I would suggest throwing a 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 However, if you switch scale factors to 1 for negative arguments, then any code calling the Furthermore, if we silently switch scale factors for |
I agree with the logic. I've updated the PR and added some more tests for this case. |
@simonbyrne any update here? |
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.
Updated. |
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) |
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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 |
oops, we should sanity check deprecations in a better way...
|
Do you mean $(QuoteNode($afn)) ? I tried that but compilation of Julia fails with
|
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.
This broke
Maybe we should run it on CI to catch this kind of bug? |
It did run on CI though https://travis-ci.org/JuliaLang/julia/jobs/187530248#L745 just didn't flag an error? |
Sorry, probably a bug on my end then. I guess I used the incorrect Julia executable to build the docs. |
The fact that |
Yes, it should. Would you mind making a pull request?
|
No, I can do it! |
This rearranges the different airy functions, deprecating
airy
,airyx
andairyprime
in favour of the more specific variants (airyai
,airyaiprime
, etc.), and clarifies the docs of each remaining function. Fixes #17032.