Bug 578 - [contribution] ZipfVariable
: [contribution] ZipfVariable
Status: RESOLVED FIXED
: ns-3
simulation core
: pre-release
: All All
: P5 normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-26 13:48 EDT by
Modified: 2009-06-24 01:48 EDT (History)


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 From 2009-05-26 13:48:48 EDT
Created an attachment (id=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 From 2009-05-27 09:51:58 EDT -------
Created an attachment (id=446) [details]
Patch, new version

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

The patch done against che correct version of ns-3
------- Comment #3 From 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 From 2009-05-27 13:54:59 EDT -------
Created an attachment (id=448) [details]
Patch with correct copy constructor

This version has a correct copy constructor
------- Comment #5 From 2009-05-28 10:27:53 EDT -------
(In reply to comment #4)
> Created an attachment (id=448) [details] [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 From 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 From 2009-05-28 14:10:22 EDT -------
Created an attachment (id=449) [details]
Patch with (hopefully) correct coding style
------- Comment #8 From 2009-05-28 14:15:09 EDT -------
Created an attachment (id=450) [details]
correct coding style
------- Comment #9 From 2009-06-03 08:50:16 EDT -------
Once we get a validation of the implementation, I'm fine with accepting this.
------- Comment #10 From 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 From 2009-06-08 07:57:00 EDT -------
Created an attachment (id=458) [details]
include header diff
------- Comment #12 From 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 From 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 From 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 From 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 From 2009-06-17 15:20:50 EDT -------
Created an attachment (id=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 From 2009-06-17 16:15:07 EDT -------
OK, I'm fine with this to be committed.

-Michele
------- Comment #18 From 2009-06-24 01:48:31 EDT -------
pushed, changeset:  c86681050541