Improve numerical precision of discrete uniform and geometric ICDFs

[gokuld]

What is this PR about?
Improve numerical precision issues for the ICDF function for the discrete geometric distribution.

Actually, this is an issue in general, wherever the ceiling function is used on a floating point number to get the ICDF value. Thus this fix for the precision issue applies to most discrete ICDF functions, and not only the Geometric distribution.

Details are outlined in this issue #6670 .

Closes #6670

Checklist

• Explain important implementation details 👆
• Make sure that the pre-commit linting/style checks pass.
• Link relevant issues (preferably in nice commit messages)
• Are the changes covered by tests and docstrings?
• Fill out the short summary sections 👇

Major / Breaking Changes

• ...

New features

• ...

Bugfixes

• Fix numerical precision issues for the discrete ICDF functions.

Documentation

• ...

Maintenance

• ...

---

📚 Documentation preview 📚: https://pymc--6671.org.readthedocs.build/en/6671/

[codecov]

Codecov Report

Merging #6671 (db0b98f) into main (9f01be2) will decrease coverage by 3.16%.
The diff coverage is 60.00%.

❗ Current head db0b98f differs from pull request most recent head 2348e4a. Consider uploading reports for the commit 2348e4a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6671      +/-   ##
==========================================
- Coverage   91.99%   88.84%   -3.16%     
==========================================
  Files          95       95              
  Lines       16016    16024       +8     
==========================================
- Hits        14734    14236     -498     
- Misses       1282     1788     +506     

Impacted Files	Coverage Δ
pymc/distributions/discrete.py	67.24% <55.55%> (-31.49%)	⬇️
pymc/testing.py	88.15% <100.00%> (-3.88%)	⬇️

... and 17 files with indirect coverage changes

[gokuld]

test_geometric and test_discrete_unif passed locally on my machine.
Let me check and get back on this.

[ricardoV94]

You have to use a switch here, ifelse will only work for scalar cases

[ricardoV94]

Make sure you pass n_samples=-1 to check_logp and the other helpers so all combinations are tried and not just a random subset

[gokuld]

Thanks, replaced ifelse with switch.

Even with n_samples=-1. pytest -v tests/distributions/test_discrete.py -k 'test_geometric or test_discrete_unif' worked locally on my machine. Perhaps it has to do something with switch / ifelse

[ricardoV94]

Do the old implementations fail with just 3 9's? I am wary of testing such an extreme value by default because it puts a high burden on future icdfs on being very accurate for extreme values. We know these things must always break at some point, specially for unbounded distributions.

Suggested change

	domain = Domain([0, 0.1, 0.5, 0.75, 0.95, 0.99, 0.9999, 1]) # Values we test the icdf at
	domain = Domain([0, 0.1, 0.5, 0.75, 0.95, 0.99, 0.999, 1]) # Values we test the icdf at

[ricardoV94]

If you want to cover the changes in this PR, you can add a small test where you just evaluate the icdf at the values that used to fail for the two distributions

[gokuld]

If you want to cover the changes in this PR, you can add a small test where you just evaluate the icdf at the values that used to fail for the two distributions

To clarify my understanding, you mean I add the specific test for those distributions that fail and not something general that applies to all future distributions right?

[gokuld]

Do the old implementations fail with just 3 9's? I am wary of testing such an extreme value by default because it puts a high burden on future icdfs on being very accurate for extreme values. We know these things must always break at some point, specially for unbounded distributions.

I added 0.9999, since as outlined in #6670,
for a Geometric distribution with p=0.99, and CDF value 0.9999 whose ICDF is expected to be 2 but it gives 3 as the result. It works fine where the ICDF of 0.99 is 1. To summarize this in a table:

p=0.99

Value	CDF	expected ICDF of the CDF	returned ICDF of the CDF	Bug
1	0.99	1	1	No
2	0.9999	2	3	Yes

[ricardoV94]

To clarify my understanding, you mean I add the specific test for those distributions that fail and not something general that applies to all future distributions right?

Yes that's what I suggest. Also note that instead of tweaking the icdf value we can tweak the domain of the distribution parameters that is tested. Probably testing more extreme Geometric p values would also have shown the error?

Regardless, a separate test is also fine

[gokuld]

Thanks @ricardoV94 .

I have decided to do this, let me know if it makes sense:

With my latest commit, I have removed the value 0.9999 in the test for ICDF to not burden all future ICDF code regarding high accuracy.

p=0.99 is already used, as defined by Unit in testing.py.

The ICDF tests will instead be covered by the self-consistency check for ICDF of discrete distributions, which I will add in this PR #6642 . (Note that the self-consistency check for ICDF of continuous distributions is already added in #6642, and the discrete self consistency check is blocked on the current PR because of this accuracy issue. Thus I will add it to #6642 after this current PR is closed.)

[ricardoV94]

Sounds good

[ricardoV94]

Thanks, replaced ifelse with switch.

Even with n_samples=-1. pytest -v tests/distributions/test_discrete.py -k 'test_geometric or test_discrete_unif' worked locally on my machine. Perhaps it has to do something with switch / ifelse

Perfect

[ricardoV94]

Is this ready to merge then?

[gokuld]

Is this ready to merge then?

Yes, in my opinion.

[ricardoV94]

Awesome, great work!

[gokuld]

Thank you! :)