Bug 797 - Enhancements to src/core/random-variable.cc/h
Enhancements to src/core/random-variable.cc/h
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: general
ns-3-dev
All All
: P5 minor
Assigned To: Michele Weigle
http://codereview.appspot.com/193060/...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-23 21:48 EST by Tommaso Pecorella
Modified: 2010-02-04 08:17 EST (History)
3 users (show)

See Also:


Attachments
CDF for alpha = 1.57 (9.44 KB, application/octet-stream)
2010-02-01 21:11 EST, Tommaso Pecorella
Details
CDF for alpha = 3.14 (4.93 KB, application/octet-stream)
2010-02-01 21:11 EST, Tommaso Pecorella
Details

Note You need to log in before you can comment on or make changes to this bug.
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.