-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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: Add examples to scipy.special docstrings #9055
DOC: Add examples to scipy.special docstrings #9055
Conversation
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.
Thanks for looking at this. I've made a few comments.
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.
Thanks for looking at this. I've made a few comments.
scipy/special/basic.py
Outdated
@@ -754,20 +754,97 @@ def riccati_yn(n, x): | |||
|
|||
|
|||
def erfinv(y): | |||
"""Inverse function for erf. | |||
"""Inverse error function. |
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.
erf
should appear in this description on the fist line somewhere. Perhaps
Inverse of the error function erf.
scipy/special/basic.py
Outdated
@@ -754,20 +754,97 @@ def riccati_yn(n, x): | |||
|
|||
|
|||
def erfinv(y): | |||
"""Inverse function for erf. | |||
"""Inverse error function. | |||
This function compute the inverse error function. |
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.
"computes"
scipy/special/basic.py
Outdated
"""Inverse function for erf. | ||
"""Inverse error function. | ||
This function compute the inverse error function. | ||
Inverse error function is satisfying :math:`erf(erf^{-1}(x))=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.
The inverse error function satisfies :math:`erf(erf^{-1}(x))=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.
do we need :math:
for this? erf(erfinv(x)) = x
is enough in my opinion
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.
erf
should be \mathrm{erf}
within :math: to get it to typeset properly, so it's probably indeed better not write it in math mode.
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.
scipy/special/basic.py
Outdated
Parameters | ||
---------- | ||
y : ndarray | ||
Argument at which to evaluate for (-1, 1) |
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.
What does evaluate for (-1, 1)
intended to indicate here? Is the domain the open interval (-1, -1)
? Or perhaps the closed interval [-1, 1]
?
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.
evaluate for (-1, 1)
means the domain the open interval from -1 to 1.
Should I depict more clearly? Could you recommend the expressions?
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.
Saying "evaluate for" itself is not very clear, that's not a construct in English that I recognize. (The function is not evaluated for the whole interval, but for given values in it.)
You could e.g. write "Argument at which to evaluate. Domain: (-1, 1)" or something similar.
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.
Thanks @pv .
I will revise this document.
scipy/special/basic.py
Outdated
Returns | ||
------- | ||
erfinv : ndarray | ||
Value of erfinv(y0), ..., erfinv(yn) |
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 array like object y
may have many dimensions, not just be a single array, and the indices would run from 0
to n-1
. You could just write this as
The inverse erf of y, element-wise
.
scipy/special/basic.py
Outdated
Returns | ||
------- | ||
erfcinv : ndarray | ||
Value of erfcinv(y0), ..., erfcinv(yn) |
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.
See above comment on return from erfinv
.
scipy/special/basic.py
Outdated
2) evaluating a ndarray | ||
|
||
>>> from scipy import special | ||
>>> import numpy as np |
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.
import numpy not needed
scipy/special/basic.py
Outdated
Parameters | ||
---------- | ||
nt : int | ||
Argument at which to evaluate |
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.
Perhaps The number of zeros to compute
?
scipy/special/basic.py
Outdated
Returns | ||
------- | ||
zeros of erf(nt) : ndarray (complex) | ||
Complex value of zeros of erf(nt) |
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.
Perhaps A complex zero of
erf`
>>> from scipy import special | ||
>>> special.erf_zeros(1) | ||
array([1.45061616+1.880943j]) | ||
|
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.
Perhaps add a line showing that erf
of this value is indeed (close to) zero.
>>> special.erf(special.erf_zeros(1))
array([4.95159469e-14-1.16407394e-16j])
revised a document (@pvanmulbregt requested changes) |
Thanks for implementing the changes by the way. Sometimes our comments can pile up pretty quick during the reviews 😃 |
Yes @ilayn! It is really hard to catch up a speed of comments😆. |
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.
@akahard2dj Thanks for the quick turn-around. Here are a few more comments.
"""Inverse function for erf. | ||
"""Inverse of the error function erf. | ||
|
||
Computes the inverse of the error function. |
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.
erf
is a function from C
to C
, and this is a many-to-one mapping as indicated by erf_zeros
. So this probably needs a little more explanation. Perhaps something like:
"erf
is a complex-valued function defined in the complex plane but restricting erf
to the real line maps [-infty, infty]
to [-1, 1]
in a 1-1 manner. erfc
is the inverse to erf
, defined on just the real interval [-1, 1]
. If presented a complex number, erfinv
raises a TypeError. If presented a real number outside [-1, 1]
, it returns one of +infty
or -infty
."
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.
ok. I also feel to descript the erfinv
.
I will revise the explanation of erfinv
based on your comment.
scipy/special/basic.py
Outdated
Parameters | ||
---------- | ||
y : ndarray | ||
Argument at which to evaluate. Domain: (-1, 1) |
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.
I think you can specify the Domain as the closed interval [-1, 1]
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.
As far as I know, the domain of the erf
is [-inf, inf] and the range is (-1, 1).
That means the domain and range of the inverse of erf
is the opposite case, domain -> range/ range-> domain.
And wiki says that The inverse error function is usually defined with domain (−1,1)
.
Could you check the domain of the erf
?
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.
Yes, there is the mathematical erf() and the implemented erf
. The implemented erf
seems to include +/I inf
in its domain, and erfinv
returns it as part of its domain.
In [88]: erf([-np.inf, np.inf])
Out[88]: array([-1., 1.])
In [91]: erfinv(np.array([-1, 1]))
Out[91]: array([-inf, inf])
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.
I understand your comment. I just think about erf
focused on mathematical base.
It seems to be right an implementation point of view.
I will revise this part from an open domain to a closed domain.
"""Inverse function for erfc. | ||
"""Inverse of the complementary error function erfc. | ||
|
||
Computes the inverse of the complementary error function erfc. |
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.
Same caveats as for erfinv
regarding complex values and domain.
scipy/special/basic.py
Outdated
Parameters | ||
---------- | ||
y : ndarray | ||
Argument at which to evaluate. Domain: (0, 2) |
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 Domain is the closed interval [0, 2]
?
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.
Please see comments on description above.
Could you check the domain of erfcinv
also?
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.
Same as above, the accepted domain is at least [0, 2]
.
array([ inf, 0.9061938 , 0.59511608, 0.37080716, 0.17914345, | ||
-0. , -0.17914345, -0.37080716, -0.59511608, -0.9061938 , | ||
-inf]) | ||
|
||
""" | ||
return -ndtri(0.5*y)/sqrt(2) | ||
|
||
|
||
def erf_zeros(nt): | ||
"""Compute nt complex zeros of error function erf(z). | ||
|
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.
Which zeros is erf_zeros
returning? The first nt
zeros? Ordered how? Does it return all zeros, or just some special subset of the zeros? I'm wondering if it just returns zeros in the first quadrant, and that if z
is a zero, then so is -z
and perhaps also +/- conj(z)
? If so, stating that would be useful to understand what it is doing.
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.
erf_zeros
returns of the locations of the zero. that means that erf_zeros
calculates the locations of the zero with respect to |z| -> inf.
erf_zeros returns the location of the erf x > 0. because of symmetry w(-x + iy) = conj(w(x + iy)), w(z) is defined by exp(-z^2)*erfc(-iz), it is sufficient to consider only x>0.
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.
Is it the case that erf(-z) = erf(z)
and erf(conj(z)) = conj(erf(z))
? If so, then each zero generates a total of 4 zeros {z, -z, iz, -iz}
[Correction: This should have been {z, -z, conj(z), -conj(z)}
].
Perhaps the following?
"""Compute the first nt
zero in the first quadrant, ordered by absolute value.
Zeros in the other quadrants can be obtained by using the symmetries erf(-z) = erf(z)
and erf(conj(z)) = conj(erf(z))
.
"""
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.
In that case, to calculate the location of the root of erf
in complex region, it is not using the erf
itself.
As I previously mentioned before comment, the function w(z)
defined by w(z) = exp(-z^2)*erfc(-z)
is used for calculating.
In general, erf
in complex plane has a y symmetry.
I also attach a reference site for calculating the root loc of erf
below.
https://www.ams.org/journals/mcom/1973-27-122/S0025-5718-1973-0326991-7/S0025-5718-1973-0326991-7.pdf
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.
My interpretation of the referenced paper ["Complex Zeros of the Error Function and of the Complementary Error Function" by Fettis, Caslin & Kramer] is that it was mainly concerned with deriving zeros of erfc(z)
using w(z)
, not zeros of erf(z)
.
DLMF https://dlmf.nist.gov/7.13#i lists the zeros of erfc(z)
with symmetry {z, -z, conj(z), -conj(z)}
. The routine CERZO
in special/specfun/specfun.f
appears to use the first two terms of that equation 7.13.1 to generate an estimate of the n-th root of erfc(z)
and then uses Newton's method to refine it. I don't think that w(z)
appears anywhere in this calculation.
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.
Let's put together the current conversation.
- the definition of
w
inerfinv()
,erfcinv()
: complex number (x, iy) - there is no definition in
erf_zeros()
After reading the contents of the conversation again, it seems like the subject of the story was different.
I have not written any description in erf_zeros()
.
As you already know, the w(z)
of the comment is a description of the referenced paper.
Actually, the erf_zeros()
is using the iteration method for calculating a zeros by CERZO
routine.
There are four complex numbers for each zeros point of erf()
because the direction of the solution is xy-symmetry as you say.
The y-symmetry I have mentioned is that the erf()
function itself is y-symmetric in the complex domain,
and the solution passing zeros is xy-symmetry.
Back to the first question
Which zeros is
erf_zeros
returning? The firstnt
zeros? Ordered how? Does it return all zeros, or just some special subset of the zeros? I'm wondering if it just returns zeros in the first quadrant, and that ifz
is a zero, then so is-z
and perhaps also+/- conj(z)
? If so, stating that would be useful to understand what it is doing.
As you already know the answer, there are four positions {z, -z, conj(z), -conj(z)} because xy-symmetry
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.
@akahard2dj Your PR adds a lot to really useful documentation to erf_zeros
with Parameters
, Returns
and Examples
sections. The only thing missing from the docstring now is a usable explanation of what the function does. I had to dig quite a while to find the answers to my questions. My suggestion is still to replace the first sentence in the docstring with
"""Compute the first nt
zero in the first quadrant, ordered by absolute value.
Zeros in the other quadrants can be obtained by using the symmetries erf(-z) = erf
(z) and erf(conj(z)) = conj(erf(z))
.
"""
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.
Sure. No problem.
I have also worried about the content for adding a description of erf_zeros()
.
Your suggestion very good and valuable explanation for erf_zeros()
.
I will add a docstring yours in my commit.
Thank you so much @pvanmulbregt.
scipy/special/basic.py
Outdated
Returns | ||
------- | ||
Complex zeros of erf(nt) : ndarray (complex) | ||
Complex value at which zeros of erf(nt) |
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.
See comments on description above, which will likely impact how you phrase the Returns.
>>> from scipy import special | ||
>>> special.erf_zeros(1) | ||
array([1.45061616+1.880943j]) | ||
>>> special.erf(special.erf_zeros(1)) |
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.
Perhaps add a comment here to explain why this step is here.:
Check that erf
is (close to) zero for the value returned by erf_zero
.
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.
Yes! I will explain why this example is used.
revised a document for a pvanmulbregt requested changed.
This looks pretty close. @akahard2dj would be great if you could address the last couple of comments. |
It was good to have a valuable discussion in this issue. |
do you mean to say you did address the comments already, or did you mean to push a new commit? |
this looks close for me, too. |
I think if the last bit of comments are addressed this is good to go. |
Thanks @akahard2dj! |
updates an example of docstrings following a guideline