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: clarify the definition of the pdf of stats.fisk #14003

Merged
merged 4 commits into from
Feb 26, 2022

Conversation

Mazensayed91
Copy link
Contributor

Reference issue

What does this implement/fix?

Additional information

@Mazensayed91 Mazensayed91 changed the title This to make the docstring for the definition for the pdf for 'fisk' more clearadded another definition for the pdf for 'fisk' This to make the docstring for the definition for the pdf for 'fisk' more clear May 8, 2021
@Mazensayed91 Mazensayed91 changed the title This to make the docstring for the definition for the pdf for 'fisk' more clear Doc: Make the docstring for the definition for the pdf for 'fisk' more clear May 8, 2021
@Mazensayed91
Copy link
Contributor Author

This related to #13996 issue.
I just added another definition for the fisk pdf

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks @Mazensayed91. Although per Matt's comment, we should switch the two formulas. Have the one without the minus as the default one.

Also there is a comment about this being also relevant for Burr. If this is correct, we could fix this here as well.

@tupui tupui added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats labels May 8, 2021
Made the positive formula as the default one, also added the same positive formula for Burr pdf
@Mazensayed91
Copy link
Contributor Author

Thanks @tupui for the modifications, I've modified the request to fit them

f(x, c, d) = c d x^{c - 1} / (1 + x^{c})^{d + 1}

Also equivalent to:

.. math::

f(x, c, d) = c d x^{-c - 1} / (1 + x^{-c})^{d + 1}
Copy link
Member

@ev-br ev-br May 8, 2021

Choose a reason for hiding this comment

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

These two formulas are only equivalent for d=1, i.e. for the fisk distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the pdf formula for burr distribution, so the first one is the right one ?

image

Copy link
Member

Choose a reason for hiding this comment

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

I agree, looks like the first one is the right one. Also, if we're using LaTeX anyway, let's use \frac and make it look identical to how the pdf is given on Wikipedia.

Copy link
Contributor

@mdhaber mdhaber Feb 23, 2022

Choose a reason for hiding this comment

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

It looks like the second one, the original one, is the intended one for the burr distribution.

This is the PDF corresponding to the third CDF given in Burr's list;
specifically, it is equation (11) in Burr's paper [1]_

This is:
image

Taking the derivative and changing the symbols used for the parameters, it turns into:
image

This is what the code implements when x != 0:

        output = _lazywhere(x == 0, [x, c, d],
                   lambda x_, c_, d_: c_ * d_ * (x_**(c_*d_-1)) / (1 + x_**c_),
                   f2 = lambda x_, c_, d_: (c_ * d_ * (x_ ** (-c_ - 1.0)) /
                                            ((1 + x_ ** (-c_)) ** (d_ + 1.0))))

(I'll leave simplifying that to another time...)

It seems that the other one is burr12.

@rgommers rgommers changed the title Doc: Make the docstring for the definition for the pdf for 'fisk' more clear DOC: clarify the definition of the pdf of stats.fisk May 26, 2021

f(x, c) = c x^{c-1} (1 + x^{c})^{-2}

Also equivalent to:
Copy link
Member

Choose a reason for hiding this comment

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

This is actually confusing, without reading the linked issue it was non-obvious that this wasn't a mistake. I suggest the following rephrase: Please note that the above expression can be transformed into the following one, which is also commonly used:

@mdhaber
Copy link
Contributor

mdhaber commented Feb 26, 2022

In case others would like to check, here are comparisons of how the documentation now renders VS Wikipedia.

Fisk

Wikipedia (log-logistic distribution)
image

SciPy (fisk)
image

Burr Type III

Wikipedia (Dagum distribution)
image

It is not obvious how the CDF would turn into the PDF above or how the PDF shown here would be equivalent to the PDF in SciPy. (Sympy cannot confirm it, so I'm not even sure if the PDF on Wikipedia is correct.) But it is more obvious that the derivative of the CDF above, which is the same as that in Burr's paper, is equivalent to the PDF used in SciPy.

from sympy import symbols, diff, simplify
x, a, p, c, d = symbols('x, a, p, c, d', real=True, positive=True)
F = (1 + x**(-a))**(-p)  # Wikipedia CDF
f = diff(F, x).subs(a, c).subs(p, d)  # Wikipdia deriviative of CDF, SciPy Notation
f_s = c*d*(x**(-c-1))/(1+x**(-c))**(d+1)  # SciPy PDF
print(simplify(f - f_s))  # 0

SciPy
image

Burr Type XII

Wikipedia ("Burr Distribution")
image

SciPy (burr12)
image

I see that it says burr instead of burr12 above; I'll fix that separately rather than waiting for CI to re-run here. This typo was not introduced in this PR.

I'm going to go ahead and merge this, but please let me know if you notice anything that the updates did not catch.

@mdhaber
Copy link
Contributor

mdhaber commented Feb 26, 2022

Thanks @Mazensayed91 and reviewers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants