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

Gamma collab #3890

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Conversation

jaketrookman
Copy link
Contributor

Work in progress of implementing Gamma function distribution into the random module
Need to look into setting seed, linter error, testing failure

@ajpotts
Copy link
Contributor

ajpotts commented Dec 3, 2024

@ajpotts ajpotts requested review from drculhane and ajpotts December 17, 2024 17:43
@ajpotts ajpotts mentioned this pull request Dec 19, 2024
var b = kArg - 1/3;
var c = 1/sqrt(9 * b);
var X = 0.0;
var V = -1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the bug. var V = -1.0; needs to go inside the while loop. Otherwise, it does V * V * V over and over again which eventually goes to infinity.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a first analysis, it looks like the V = -1.0 line was inserted so that the "while V <= 0" loop almost immediately below will execute at least once. But that could be done via a do-while loop, as is done in the python.

@drculhane
Copy link
Contributor

drculhane commented Dec 23, 2024

On further analysis, this looks like a faithful line by line copy of the python code, assuming that certain things correspond (e.g. bitgen_t in python to randomStream in chapel). The only difference I see is the V = -1 line that Amanda commented on above.

On further further analysis, I did my best to test this as a standalone function, and so far I'm seeing no differences between the while-do and do-while versions.

I was able to produce some anomalous seed behavior. The short description: my while-do and do-while versions almost always produce the same results given the same seeds. But sometimes they don't. Maybe when the holidays are over and we're all back in the office, I can show you what I mean.


}
else {
var b = kArg - 1/3;
Copy link
Contributor

Choose a reason for hiding this comment

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

1/3 needs to be 1/3.0 for correct floating point division. Same for any other computations in this function. This is
Screenshot from 2024-12-19 16-25-04
causing the distribution to be slightly off.

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.

4 participants