Skip to content

Conversation

dschmitz89
Copy link
Contributor

@dschmitz89 dschmitz89 commented Feb 11, 2024

Reference issue

Related to #19953 and #7168

What does this implement/fix?

  1. Fixes the docstring of these functions that were in the wrong order (see here)
  2. Adds examples to the docstring

@github-actions github-actions bot 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 Feb 11, 2024
@dschmitz89 dschmitz89 requested a review from ilayn February 11, 2024 07:46
@ilayn
Copy link
Member

ilayn commented Feb 11, 2024

Nice. By the way, please don't forget the possibility that I might have screwed up the argument order while writing the wrappers :) I paid attention trying not to touch anything on the scipy side but who knows. But then again it would break more stuff.

@lucascolley lucascolley added this to the 1.13.0 milestone Feb 11, 2024
@dschmitz89
Copy link
Contributor Author

No worries about the rewrite, the docstring was apparently broken for many years. The example returns the same output using scipy 1.10.1. Good that the rewrite forces us to look into these functions more ;)

@ilayn
Copy link
Member

ilayn commented Feb 11, 2024

FFT errors are not unrelated I think. But gcc one seems legit error. That should be a cdflib mistake if the remaining is passing. I'll check.

Could you also remove the entries in line 13, 14 if the tests are added?

@dschmitz89
Copy link
Contributor Author

The test still fails: https://github.com/scipy/scipy/actions/runs/7860753553/job/21448286231?pr=20069

It only shows up when the full test suite is run as the cdflib tests are marked as slow.

I do not understand the origin of the test failure. For some reason,the machinery picks up the wrong parameters to test against each other.

@ilayn
Copy link
Member

ilayn commented Feb 11, 2024

I wish I can understand the testing mechanism there but it is a bit too much of dependency injection for me. I feel like this is about the _assert_inverts but can't figure it out due to my lack of mpmath and special knowledge .

@dschmitz89
Copy link
Contributor Author

dschmitz89 commented Feb 12, 2024

I wish I can understand the testing mechanism there but it is a bit too much of dependency injection for me. I feel like this is about the _assert_inverts but can't figure it out due to my lack of mpmath and special knowledge .

Could not agree more. It looks easy to use in principle even without special function expertise but if something fails, I find it hard to debug. @person142: would you have time to have a look at this?

@dschmitz89 dschmitz89 changed the title MAINT/DOC: special.nrdtrimn/nrdtrisd docstring fixes and tests MAINT/DOC: special.nrdtrimn/nrdtrisd docstring fixes Feb 17, 2024
[docs only]
[docs only]
@dschmitz89
Copy link
Contributor Author

I removed the new tests and leave them for a separate PR. This now only solves the documentation issue.

@dschmitz89
Copy link
Contributor Author

Sorry for that rookie mistake. Looks good now. I wish the linter/refguide check would catch that (would numpydoc validation help?).

@lucascolley
Copy link
Member

would numpydoc validation help?

I don't think so based on https://numpydoc.readthedocs.io/en/latest/validation.html#built-in-validation-checks

Comment on lines +11131 to -11111
mn : scalar or ndarray
The mean of the normal distribution.
p : array_like
CDF values, in range (0, 1].
x : array_like
Quantiles, i.e. the upper limit of integration.
mn : scalar or ndarray
The mean of the normal distribution.
Copy link
Contributor

Choose a reason for hiding this comment

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

The signature above says nrdtrisd(p, x, mn, out=None), but here the order is mn, p, x`. I think these should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 done

Copy link
Contributor

@steppi steppi left a comment

Choose a reason for hiding this comment

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

Looks good now. Also, I noticed that nrdtrisd returns garbage for infeasible inputs, say when you have a CDF value > 0.5 but x is less than the mean. e.g nrdtrisd(0, 0.6, -1) = -3.9471538755427473. This is because nrdtrisd is really finding a scale parameter, not a standard deviation, and a negative scale gives a reflection of the original distribution. This should be addressed at some point too in a future PR. Either by changing the behavior, or documenting the existing behavior.

@steppi steppi merged commit 8fe0529 into scipy:main Feb 18, 2024
@steppi
Copy link
Contributor

steppi commented Feb 18, 2024

Thanks @dschmitz89!

@dschmitz89 dschmitz89 deleted the normal_dist_cdflib_tests branch November 29, 2024 14:05
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 maintenance Items related to regular maintenance tasks scipy.special
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants