Bug 578 - [contribution] ZipfVariable
[contribution] ZipfVariable
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: core
pre-release
All All
: P5 normal
Assigned To: Michele Weigle
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-26 13:48 EDT by Francesco Malandrino
Modified: 2009-06-24 01:48 EDT (History)
5 users (show)

See Also:


Attachments
The patch (2.29 KB, patch)
2009-05-26 13:48 EDT, Francesco Malandrino
Details | Diff
Patch, new version (45.88 KB, patch)
2009-05-27 09:51 EDT, Francesco Malandrino
Details | Diff
Patch (for real) (2.63 KB, patch)
2009-05-27 09:55 EDT, Francesco Malandrino
Details | Diff
Patch with correct copy constructor (2.23 KB, patch)
2009-05-27 13:54 EDT, Francesco Malandrino
Details | Diff
Patch with (hopefully) correct coding style (2.24 KB, patch)
2009-05-28 14:10 EDT, Francesco Malandrino
Details | Diff
correct coding style (2.27 KB, patch)
2009-05-28 14:15 EDT, Mathieu Lacage
Details | Diff
include header diff (2.93 KB, patch)
2009-06-08 07:57 EDT, Mathieu Lacage
Details | Diff
Validation plot for Zipf RNG with n=100. shape=1 (4.06 KB, image/png)
2009-06-17 15:20 EDT, Craig Dowell
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Francesco Malandrino 2009-05-26 13:48:48 EDT
Created attachment 445 [details]
The patch

The attached patch adds a new ZipfVariable random variable.
The implementation is likely suboptimal, but it may still be useful.
Comment 1 Francesco Malandrino 2009-05-27 09:51:58 EDT
Created attachment 446 [details]
Patch, new version

This new version adds a default constructor. Additionally, it compiles correctly also in optimized mode.
Comment 2 Francesco Malandrino 2009-05-27 09:55:22 EDT
Created attachment 447 [details]
Patch (for real)

The patch done against che correct version of ns-3
Comment 3 Michele Weigle 2009-05-27 13:20:21 EDT
This looks fine, though I didn't check the math -- I assume it's correct.   Just to check, can you generate a PDF or CDF using this code and compare it against the theoretical PDF/CDF?

I do have a question about the Copy constructor:
RandomVariableBase* ZipfVariableImpl::Copy () const
{
  return new ZipfVariableImpl (m_n, m_alpha);
}

All of the other random variables have Copy constructors like this:
RandomVariableBase* NormalVariableImpl::Copy() const
{
  return new NormalVariableImpl(*this);
}

Why is this one different? 
Comment 4 Francesco Malandrino 2009-05-27 13:54:59 EDT
Created attachment 448 [details]
Patch with correct copy constructor

This version has a correct copy constructor
Comment 5 Mathieu Lacage 2009-05-28 10:27:53 EDT
(In reply to comment #4)
> Created an attachment (id=448) [details]
> Patch with correct copy constructor
> 
> This version has a correct copy constructor
> 

This version still has incorrect coding style: can you fix it before merging ?

i.e., 

for ()
  //do stuff

should be:

for ()
  {
    // do stuff
  }

yes, even for one-line for loops. see http://www.nsnam.org/codingstyle.html

Comment 6 Tom Henderson 2009-05-28 10:35:03 EDT
(In reply to comment #3)
> This looks fine, though I didn't check the math -- I assume it's correct.  
> Just to check, can you generate a PDF or CDF using this code and compare it
> against the theoretical PDF/CDF?

Craig just did some chi-squared testing of our other distributions, so I suspect he could easily check this one too.
Comment 7 Francesco Malandrino 2009-05-28 14:10:22 EDT
Created attachment 449 [details]
Patch with (hopefully) correct coding style
Comment 8 Mathieu Lacage 2009-05-28 14:15:09 EDT
Created attachment 450 [details]
correct coding style
Comment 9 Michele Weigle 2009-06-03 08:50:16 EDT
Once we get a validation of the implementation, I'm fine with accepting this.
Comment 10 Mathieu Lacage 2009-06-08 07:52:22 EDT
(In reply to comment #9)
> Once we get a validation of the implementation, I'm fine with accepting this.
> 

I can easily generate data for this implementation. Can you generate reference data I can check this against ?
Comment 11 Mathieu Lacage 2009-06-08 07:57:00 EDT
Created attachment 458 [details]
include header diff
Comment 12 Tom Henderson 2009-06-08 09:16:23 EDT
(In reply to comment #9)
> Once we get a validation of the implementation, I'm fine with accepting this.
> 

FYI, I asked Craig to run this through his chi-squared test framework and gsl, and report back here.
Comment 13 Craig Dowell 2009-06-15 20:03:59 EDT
GSL does not provide a Zipf/zeta distribution to use as a known-good PDF.

I can probably get the needed info out of Mathematica tomorrow; but this will be a hand-crafted very much special case validation test.

Anyone know of an (authoritative) open-source Zipf/zeta PDF and CDF function I can get my hands on?


Comment 14 Michele Weigle 2009-06-16 08:05:15 EDT
I found something at NIST.
http://www.itl.nist.gov/div898/software/dataplot/refman2/auxillar/zippdf.htm

Looks like they have a Zipf distirbution in their Dataplot application:
http://www.itl.nist.gov/div898/software/dataplot/homepage.htm

Hope this helps.

-Michele
Comment 15 Craig Dowell 2009-06-16 15:05:16 EDT
Thanks Michele, ZIPCDF from DataPlot is exactly what I needed.

Heh, I love this.  Fortran-77 code with the original bleeding-edge version released by James J. Filliben in 1978.

Still useful (and runnable!) after all these years.












Comment 16 Craig Dowell 2009-06-17 15:20:50 EDT
Created attachment 470 [details]
Validation plot for Zipf RNG with n=100. shape=1

It took a little work, but I finally put together a simple validation test for this RNG.  This is by no means a comprehensive test, but the Zipf RNG appears to work nicely as advertised for N=100, alpha=1.  I don't plan on doing any more testing.

See attached PNG for a pretty picture.
Comment 17 Michele Weigle 2009-06-17 16:15:07 EDT
OK, I'm fine with this to be committed.

-Michele

Comment 18 Craig Dowell 2009-06-24 01:48:31 EDT
pushed, changeset:  c86681050541