Bug 797

Summary:	Enhancements to src/core/random-variable.cc/h
Product:	ns-3	Reporter:	Tommaso Pecorella <tommaso.pecorella>
Component:	general	Assignee:	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 UTC

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 UTC

Michele said that she'd review this.

Comment 2 Michele Weigle 2010-02-01 08:22:33 UTC

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 UTC

(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 UTC

Created attachment 745 [details]
CDF for alpha = 1.57

Comment 5 Tommaso Pecorella 2010-02-01 21:11:51 UTC

Created attachment 746 [details]
CDF for alpha = 3.14

Comment 6 Tommaso Pecorella 2010-02-01 21:44:18 UTC

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 UTC

This looks good, please feel free to push the changes yourself.

Comment 8 Tommaso Pecorella 2010-02-04 08:17:00 UTC

Thanks,

the patch has been pushed.