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

test: add check for derivative of CDF to ensure it matches PDF #320

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

Qazalbash
Copy link
Contributor

I have used finite difference approximation of derivatives. It seems like there are some numerical inaccuracies!

@YeungOnion
Copy link
Contributor

YeungOnion commented Jan 26, 2025

Hmm, I see that this appears for

  • Beta
  • Student T
  • Uniform

I believe the Uniform one makes sense for the error when there's an evaluation over the stepped edges.

The other two CDF rely on some special functions which could have insufficient precision for finite differences over some steps. That is,

For δ, the precision error in CDF evaluation and ε, the roundoff error from subtraction and representation, and step size h, we have some error from finite difference techniques $h^2$ and from floating point $\epsilon/h$ and the second term is contributing significantly.

Could you rule out that possibility? Note that our tests for special functions are typically checking at least 12 digits1.

Let me know if you need any other guidance or advice.

EDIT 1: fix typo for footnote

Footnotes

  1. referring to a loose notion of the number of base 10 digits, but details are in implementation and our use of RelativeEq and AbsDiffEq

@Qazalbash
Copy link
Contributor Author

It is resolved by comparing,

$$ |F(x+h)-F(x-h)-2hf(x)|<\epsilon $$

instead of,

$$ \left|\frac{F(x+h)-F(x-h)}{2h}-f(x)\right|<\epsilon $$

seems like we lost precision due to floating point division.

@Qazalbash
Copy link
Contributor Author

I have set at least one of the checks should pass, i.e. integration of pdf should be equal to cdf or derivative of cdf should be equal to pdf!

@YeungOnion
Copy link
Contributor

I think the multiplication is a good solution!

I made some comments, one for an error chaining idiom, and the other on docs.

I'm against using catch_unwind unless there's not a clear alternative. However, this is a test and not the lib code, so catching unwinding thrown by anything other than the assertion would be fairly strange behavior. So I'll leave it up to you if that change to the check functions will be in this PR, I know that you also want to get #317 through as well.

@Qazalbash Qazalbash requested a review from YeungOnion January 29, 2025 06:29
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.27%. Comparing base (a8fe65c) to head (60c5462).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
src/distribution/internal.rs 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
+ Coverage   93.81%   94.27%   +0.45%     
==========================================
  Files          53       58       +5     
  Lines       11996    12974     +978     
==========================================
+ Hits        11254    12231     +977     
- Misses        742      743       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@YeungOnion YeungOnion merged commit 71e010b into statrs-dev:master Jan 29, 2025
10 checks passed
@Qazalbash Qazalbash deleted the d-cdf-equals-pdf branch January 29, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants