Handmade distributions have the option not to check for validity

[Maelstrom6]

References to other Issues or PRs

Fixes #19959

Brief description of what is fixed or changed

(stats module)

Some handmade distributions raised errors saying that the density function did not integrate to 1. This was simply because the integrate function did not integrate to 1 exactly, but rather an expression that is simplifies to 1. I have given the option for the user to allow SymPy to check the validity.

Other comments

On request, I will add similar functionality for drv_types.py, frv_types.py and possibly some vector and matrix handmade distributions.

I can also add a .simplify() after integrating.

Release Notes

NO ENTRY

[sympy-bot]

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

• No release notes entry will be added for this pull request.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #19959

#### Brief description of what is fixed or changed

(stats module)

Some handmade distributions raised errors saying that the density function did not integrate to 1. This was simply because the `integrate` function did not integrate to 1 exactly, but rather an expression that is simplifies to 1. I have given the option for the user to allow SymPy to check the validity.

#### Other comments

On request, I will add similar functionality for `drv_types.py`, `frv_types.py` and possibly some vector and matrix handmade distributions.

I can also add a `.simplify()` after integrating.

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

[codecov]

Codecov Report

Merging #20120 into master will decrease coverage by 0.007%.
The diff coverage is 89.473%.

@@              Coverage Diff              @@
##            master    #20120       +/-   ##
=============================================
- Coverage   75.854%   75.847%   -0.008%     
=============================================
  Files          671       671               
  Lines       173815    173788       -27     
  Branches     41046     41065       +19     
=============================================
- Hits        131847    131814       -33     
+ Misses       36221     36220        -1     
- Partials      5747      5754        +7     

[Smit-create]

Thanks for the PR!
This looks good to me. I request you to add the same of drv and frv.

[Maelstrom6]

joint_rv_types.py has a completely different setup for JointRV. Currently, JointRV does not check for validity of the distribution.

[Smit-create]

@czgdp1807 @oscarbenjamin Please have a look if this can be merged.

[oscarbenjamin]

We shouldn't have a section called "Other parameters".

The expected sections are listed here:
https://docs.sympy.org/latest/documentation-style-guide.html#docstring-sections

[Maelstrom6]

I have added it under the "Parameters" section in that case.

I found an error in the guide: they state that one needs an extra section for Returns but Returns is not part of the list of the valid sections. Probably not a big deal though.

[oscarbenjamin]

I'm not sure it's necessary to have a check at all but if it is then shouldn't we just fix the check so it doesn't incorrectly return False?

We could use e.g. Eq(expr, 1) is S.false rather than expr == 1.

[Maelstrom6]

I'm not sure it's necessary to have a check at all but if it is then shouldn't we just fix the check so it doesn't incorrectly return False?

We could use e.g. Eq(expr, 1) is S.false rather than expr == 1.

I'll add that as well. But I fear that for some complicated distributions, Integrate might not be able to be simplified, and users will be stuck with the situation now.

Edit: Never mind. I see that you changed the condition now. But maybe Integrate could still hang. Users could be stuck on the check for ever.

[oscarbenjamin]

It's probably better not to use integrate by default anyway. Checking should be something that a user chooses to do if it isn't guaranteed to be cheap. Or perhaps there could be an API that "normalises" a pdf to produce a distribution.

[Maelstrom6]

Checking should be something that a user chooses to do if it isn't guaranteed to be cheap.

This is the goal of this PR. My handmade distributions had this problem.

Or perhaps there could be an API that "normalises" a pdf to produce a distribution.

I think this could be done by the users on their own since its only 1 line of work:
new_pdf = old_pdf/integrate(old_pdf, (x, set))

[oscarbenjamin]

I think this could be done by the users on their own since its only 1 line of work:

Then we could just remove the checking. I'd rather do that than have a "check" argument.

[Maelstrom6]

What if the default was not to check? I'm sure some users indirectly rely on the check.

[oscarbenjamin]

How would they rely on the check? Doesn't it just throw an exception (incorrectly some of the time).

[Maelstrom6]

How would they rely on the check? Doesn't it just throw an exception (incorrectly some of the time).

One uses of a custom RV is so that a developer can design an API for clients that can input their own distributions. The developer might assume that these checks are done by sympy and has a try-catch ready for incorrect user input. If we change this, their code might break since their functions are running with an invalid distribution.

Even though the checks fail for complicated distributions, they don't fail for most of the usual ones.

[oscarbenjamin]

Is that a hypothetical use case or do you know of anyone doing that?

I don't think we need to worry about checking. It should be up to users to pass in valid input because sympy can not check that in general. We can provide a function for checking or otherwise an API that explicitly states that it will normalise a pdf.

[Maelstrom6]

Is that a hypothetical use case or do you know of anyone doing that?

I am the only one I know doing this. This is how I found out about the problem.

I can add that extra normalising functionality as an extra keyword arg in this PR. I can also set the default for the check to false. Should I do these two things?

[oscarbenjamin]

I'll let someone more familiar with stats and how it is used comment. My views here are just in general for sympy:

1. It's usually better to provide separate functions for things rather than having boolean flags.
2. Expensive operations like integration should not be enabled by default unless strictly necessary.
3. We should not use **kwargs when we can use an explicit keyword argument (now that Python 2 isn't supported any more)

[czgdp1807]

2. Expensive operations like integration should not be enabled by default unless strictly necessary.

Agreed.

[czgdp1807]

+1

[czgdp1807]

LGTM. Will merge tomorrow if no objections.