Bug 797

Summary: Enhancements to src/core/random-variable.cc/h
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: generalAssignee: Michele Weigle <mweigle>
Status: RESOLVED FIXED    
Severity: minor CC: mathieu.lacage, ns-bugs, tomh
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
URL: http://codereview.appspot.com/193060/show
Attachments: CDF for alpha = 1.57
CDF for alpha = 3.14

Description Tommaso Pecorella 2010-01-23 21:48:21 EST
Zipf random variable needed documentation. Truncated Exponential random variable was simply wrong (old one had a formula such as the PDF integral was not even bounded, let alone equal to one).

Code indentation was slightly non-conformant in the whole file too.

Plus, since I was at it, I added the Zeta distribution (it's just a Zipf distribution with N=infinity, but the generation method is rather different).

No changes to the interfaces or the behavior of the actual random number generators.

The changeset is in the codereview repository.
Comment 1 Tom Henderson 2010-01-29 01:34:46 EST
Michele said that she'd review this.
Comment 2 Michele Weigle 2010-02-01 08:22:33 EST
This looks good and thanks for fixing the style issues!

Can you generate a couple representative PDFs along with a reference graph to validate the new code?

Here are some typos that should be fixed.

random-variable.h:

Section @@ -282,17 +282,18 @@
internal -> interval
b/(e^(\alpha \, b)-1) -> b/(e^{\alpha \, b}-1)

Section @@ -301,33 +302,33 @@
m - b/(e^(b/m)-1) -> b/e^{b/m}-1)

Section @@ -683,50 +686,87 @@
\param N the number of possible items, Must be a positive integer.
, -> .

Zeta distribution have one parameters, alpha,
-> has one parameter

There are the same changes needed in random-variable.cc as regards the latex functions in Exponential doxygen comments (replace some '(' with '{').
Comment 3 Tommaso Pecorella 2010-02-01 10:32:15 EST
(In reply to comment #2)
> This looks good and thanks for fixing the style issues!
> 
> Can you generate a couple representative PDFs along with a reference graph to
> validate the new code?

Thanks, and sure, it shouldn't be a problem.

> Here are some typos that should be fixed.
> [...]

... and I double checked those formulas as well! I must be blind to leave those typos.

Everything will be fixed in the next submission, thank you very much !

Cheers,

Tommaso
Comment 4 Tommaso Pecorella 2010-02-01 21:11:13 EST
Created attachment 745 [details]
CDF for alpha = 1.57
Comment 5 Tommaso Pecorella 2010-02-01 21:11:51 EST
Created attachment 746 [details]
CDF for alpha = 3.14
Comment 6 Tommaso Pecorella 2010-02-01 21:44:18 EST
I updated the code as for your comments, plus I did some testing.

10.000 numbers generated, alpha = 3.14 or 1.57. The CDFs are attached.

Alpha 1.57 doesn't have an expected value, while alpha = 3.14 have an expected value of 1.30111. Generated data gave 1.2978.

Quite good precision.
Comment 7 Michele Weigle 2010-02-03 19:49:04 EST
This looks good, please feel free to push the changes yourself.
Comment 8 Tommaso Pecorella 2010-02-04 08:17:00 EST
Thanks,

the patch has been pushed.