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

Add Marsaglias and Tsangs Ziggurat algorithm #266

Merged
merged 5 commits into from
Jul 1, 2014

Conversation

dusank
Copy link
Contributor

@dusank dusank commented Jul 1, 2014

For generating random variables from decreasing densities:

  • rnor - Gaussian
  • rexp - Exponential

Includes generator of MATLAB files for data analysis via histogram and Distribution Fitting App (dfittool).

MATLAB analysis looks good. 😅

@dusank
Copy link
Contributor Author

dusank commented Jul 1, 2014

rnor
rexp

@non
Copy link
Contributor

non commented Jul 1, 2014

Hi @dusank. I think the tests are failing for another reason. I'm going to confirm that these tests pass and then merge this manually.

@non
Copy link
Contributor

non commented Jul 1, 2014

I can confirm that the test failure was unrelated to this branch.

Looking at this more closely, there are three changes I'd like you to make:

  1. import from java.lang.Math instead of Math (I think referencing Math directly is currently deprecated).
  2. Make Ziggurat a class, and have object Ziggurat extends Ziggurat. Since the ziggurat object is not threadsafe and has internal mutable state, users may want (or need) to create multiple instances of this to generate gaussian values in parallel.
  3. Move the main method out of Ziggurat.scala and into its own file in a project we don't distribute, like benchmarks or examples.

What do you think? Do these make sense to you?

@dusank
Copy link
Contributor Author

dusank commented Jul 1, 2014

Fixed 1) by importing from scala.math instead.

@dusank
Copy link
Contributor Author

dusank commented Jul 1, 2014

Regarding 2)

The Ziggurat singleton "Utility" object IMHO fully suffices as is.

  • Although it indeed does incorporate some state in mutable arrays, the state (after initialization and setup) never changes during any method invocations. It is essentially write-once / read-only state.
  • The state is properly encapsulated and the public interface doesn't provide any means for its alteration neither.

IMHO: For all practical purposes we can consider the actual implementation to be threadsafe.

@non
Copy link
Contributor

non commented Jul 1, 2014

Oh, that's great. I hadn't realized it. I am pretty sure object construction is threadsafe so you're right! :)

@dusank
Copy link
Contributor Author

dusank commented Jul 1, 2014

With 1) and 2) out of the way:

Regarding 3)

  • The code is not an example, not a test, neither is it a benchmark.
  • It's probably more similar to some build report generator than any of the above.
  • It is a data/script generator for facilitating the quality analysis via an external tool (MATLAB/dfittool)

I wonder where to best place such code in the spire project tree hierarchy as well as where to output the generated files (D:\\ was obviously just a temporary quick hack).

@non
Copy link
Contributor

non commented Jul 1, 2014

Here's my opinion:

I'm reluctant to create a new top-level location just for this code. There is already some code for testing trigonometry functions on BigDecimal values (see bigtrig.scala) which is why I thought of that location. I agree that benchmark doesn't make sense, but I feel like test (since it is used to interactively test the distribution) or examples (due to the precedent) could work.

I am fine with using any existing top-level project (excluding macros and core obviously) and I would also be fine with leaving that code out -- it's up to you.

@dusank
Copy link
Contributor Author

dusank commented Jul 1, 2014

Done: 3)

So @non, how do you like the code / performance / generation quality?

Would you like to hook it up with Gaussian[A] and Exponential[A] to replace Marsaglias underperforming polar method and deprecate Generator#nextGaussian?

@non
Copy link
Contributor

non commented Jul 1, 2014

Yes, please go for it! I haven't written a benchmark for it yet, but I think this is the strategy we'll want to use. Once it's merged we can do profiling and optimize more as necessary, but it's looking really good already! 👍

non added a commit that referenced this pull request Jul 1, 2014
Add Marsaglias and Tsangs Ziggurat algorithm
@non non merged commit ad44464 into typelevel:master Jul 1, 2014
@dusank
Copy link
Contributor Author

dusank commented Jul 2, 2014

Benchmark: #270

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