Conversation
…/mgf` The geometric distribution in stdlib models the number of failures before the first success (support 0, 1, 2, ...), as documented by `pmf`, `cdf`, `mean`, and `variance`. Its MGF is therefore `p / (1 - (1-p) e^t)`. The implementation returned `p e^t / (1 - (1-p) e^t)`, which is the MGF of the number of trials (support 1, 2, ...), so every value was off by a factor of `e^t`. For example, `mgf( 0.5, 1.0 )` returned `e^0.5` instead of `1`, and the derivative at `t = 0` was `1/p` instead of `mean( p ) = (1-p)/p`. This commit fixes the JavaScript and C implementations, regenerates the test fixtures, updates the `p = 1` test, and corrects the documented equation and examples, including those in `geometric/ctor`.
|
👋 Hi there! 👋 And thank you for opening your first pull request! We will review it shortly. 🏃 💨 Getting Started
Next Steps
Running Tests LocallyYou can use # Run tests for all packages in the math namespace:
make test TESTS_FILTER=".*/@stdlib/math/.*"
# Run benchmarks for a specific package:
make benchmark BENCHMARKS_FILTER=".*/@stdlib/math/base/special/sin/.*"If you haven't heard back from us within two weeks, please ping us by tagging the "reviewers" team in a comment on this PR. If you have any further questions while waiting for a response, please join our Zulip community to chat with project maintainers and other community members. We appreciate your contribution! Documentation Links |
|
I commented on the original issue: #15780 (comment) We first need to determine whether it is actually a bug and whether we want to keep the current formulation. |
|
We should bump REQUIRE to a newer Distributions.jl version here and regenerate the fixtures from that. Would you like to give that a shot, @SajalDevX, or shall we take this PR over? Thanks for your contribution! |
…ists/geometric/mgf`
|
Sure, done! I bumped REQUIRE to Distributions 0.25.131 / julia 1.10.10 / JSON 1.10.0 and regenerated |
Resolves #15780.
Description
This pull request:
stats/base/dists/geometric/mgf(JS and C) so that it returns the MGF of the number of failures before the first success,p / (1 - (1-p) e^t). That is the convention used by the other geometric packages, such aspmf,mean, andvariance. Before this fix it returnedp e^t / (1 - (1-p) e^t), the MGF of the number of trials, so every value was off by a factor ofe^t. For example,mgf( 0.5, 1.0 )returnede^0.5instead of1, andM'(0)was1/pinstead ofmean( p ) = (1-p)/p.expectedvalues intest/fixtures/julia/{small,large}_p.json, keeping the samexandpinputs, and changes thep = 1test to expect1.geometric/mgfandgeometric/ctor.Related Issues
This pull request has the following related issues:
Questions
The original fixtures were generated with Distributions.jl 0.23.8 (per
REQUIRE). That version used the same trials-based formula, which is why the old tests passed. I didn't have Julia available, so I regeneratedexpectedwith 60-digit decimal arithmetic forp / (1 - (1-p) e^t), rounded to the nearest double. Current Distributions.jl computes the same quantity asp / (p - (1-p) expm1(t)). Should I bumpREQUIREto a newer Distributions.jl version, or would you rather regenerate the fixtures withrunner.jl?Other
test.js,test.mgf.js, andtest.factory.jsforgeometric/mgf, plusgeometric/ctor/test/test.js. With the new fixtures and the old code, 2000 fixture assertions failed in each oftest.mgf.jsandtest.factory.js. After the fix, everything passes (2016/2016 and 2014/2014; ctor 67/67). I couldn't build the native add-on, sotest.native.jswas skipped. To check the C change, I compiledsrc/main.con its own with libm stand-ins forexp,ln, andis_nan. It gives the same results as the JS version: 1.2843610871738946 for(0.2, 0.5)and 1 for(0.5, 1.0).mgf/docs/imgstill shows the old formula. I only changed the raw equation text in the README, so the SVG will need to be regenerated with the usual tooling.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
Used Claude to help analyze the code and draft the fix; I reviewed and tested the change myself.
@stdlib-js/reviewers