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/MAINT: special: Several updates for tklmbda #17267

Merged
merged 8 commits into from
Apr 21, 2023

Conversation

WarrenWeckesser
Copy link
Member

See the individual commit messages for details.

@WarrenWeckesser WarrenWeckesser added scipy.special Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org maintenance Items related to regular maintenance tasks labels Oct 20, 2022
@WarrenWeckesser
Copy link
Member Author

WarrenWeckesser commented Oct 20, 2022

Updated docstring: tklmbda

@WarrenWeckesser WarrenWeckesser added the C/C++ Items related to the internal C/C++ code base label Oct 30, 2022
* Use spaces instead of tabs.
* Alway enclose the body of an if statment in braces,
  even if it is just one line.

See PEP 7,  https://peps.python.org/pep-0007
* Add "Examples" section.
* Add "See Also" section.
* Copy-edit a bit.
@WarrenWeckesser
Copy link
Member Author

Rebased, and updated the link to the rendered docstring on CircleCI. The segfault in the CirrusCI job musllinux_amd64_test is not related to this PR.

Copy link
Contributor

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

This looks good to me - out of curiosity, was the missing bounds check creating a bug somewhere?

Also, I noticed both in the tutorial and in the docstring for the Tukey lambda distribution the formatting is still "wrong" (Tukey-Lambda instead of Tukey lambda). I am guessing those fixes can be done in a follow-up - they also look like good first issues :)

@WarrenWeckesser
Copy link
Member Author

Thanks @melissawm.

[...] was the missing bounds check creating a bug somewhere?

If we call it a bug, it is not a very serious bug. The differences would only be noticed at the ends of the support of the distribution, and they are tiny.

Here are some cases where the change makes a noticeable difference. The values at the ends of the support should be 0 and 1. With the main development branch:

In [2]: tklmbda([-1, 1], 1).tolist()
Out[2]: [7.105427357601002e-15, 0.9999999999999929]

In [3]: tklmbda([-4, 4], 0.25).tolist()
Out[3]: [7.105427357601002e-15, 0.9999999999999929]

In [4]: tklmbda([-0.125, 0.125], 8).tolist()
Out[4]: [7.105427357601002e-15, 0.9999999999999929]

With this pull request:

In [2]: tklmbda([-1, 1], 1).tolist()
Out[2]: [0.0, 1.0]

In [3]: tklmbda([-4, 4], 0.25).tolist()
Out[3]: [0.0, 1.0]

In [4]: tklmbda([-0.125, 0.125], 8).tolist()
Out[4]: [0.0, 1.0]

@dschmitz89 dschmitz89 self-requested a review April 1, 2023 06:10
Copy link
Contributor

@dschmitz89 dschmitz89 left a comment

Choose a reason for hiding this comment

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

Very illustrative examples. Nitpick: it could be helpful to add a comparison of calling the Tukey lambda CDF via special.tklmbda or scipy.stats.tukeylambda but that is not a must for me.

@WarrenWeckesser
Copy link
Member Author

@dschmitz89, good idea. I added a note at the end of the docstring. You can see the updated docstring at tklmbda

@WarrenWeckesser WarrenWeckesser added this to the 1.11.0 milestone Apr 19, 2023
@melissawm
Copy link
Contributor

This looks like a clear improvement so I'll put it in - thanks @WarrenWeckesser !

@melissawm melissawm merged commit e22bb48 into scipy:main Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org maintenance Items related to regular maintenance tasks scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants