data collection for ns-3.18

This is a revision of a previous code review issue.  More information, including a link to the proposed new manual chapter documentation, is available here:

https://www.nsnam.org/wiki/index.php/Data_Collection_Framework#July_2013_code_review

Still to do:
- test suite coverage of aggregators, helpers, probes
- write tutorial text to cover the new seventh.cc example
- CHANGES.html and RELEASE_NOTES

[Vedran Miletić, 2013-07-30 19:15:32]

Some comments. Hope this gets pushed for 3.18.

Regards,
Vedran

https://codereview.appspot.com/10974043/diff/3001/.hgignore
File .hgignore (right):

https://codereview.appspot.com/10974043/diff/3001/.hgignore#newcode44
.hgignore:44: \.plt$
Not related to DCF, but I like this very much.

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/conf.py
File doc/manual/source/conf.py (right):

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/conf.py#n...
doc/manual/source/conf.py:53: release = 'ns-3.16'
Should probably be updated.

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/index.rst
File doc/manual/source/index.rst (right):

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/index.rst...
doc/manual/source/index.rst:28: data-collection
Redundant?

https://codereview.appspot.com/10974043/diff/3001/examples/tutorial/seventh.cc
File examples/tutorial/seventh.cc (right):

https://codereview.appspot.com/10974043/diff/3001/examples/tutorial/seventh.c...
examples/tutorial/seventh.cc:264: Simulator::Destroy ();
Upon running this, I get .plt file with:

set terminal png
set output "gnuplot-helper-plot.png"
set title "Gnuplot Helper Plot"
set xlabel "X Values"
set ylabel "Y Values"

set datafile missing "-nan"

and empty .dat file. Is this intended?

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
File src/network/utils/packet-data-calculators.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
src/network/utils/packet-data-calculators.cc:37: NS_LOG_FUNCTION_NOARGS ();
Should be NS_LOG_FUNCTION (this);

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
File src/network/utils/packet-data-calculators.h (right):

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
src/network/utils/packet-data-calculators.h:40: void PacketUpdate (std::string
path, Ptr<const Packet> packet);
Missing documentation

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/adaptor.rst
File src/stats/doc/adaptor.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/adaptor.rst#n...
src/stats/doc/adaptor.rst:16: part, to generate artifacts with their
simulation's results.
Artifacts could also mean errors, perhaps a better word could be used here, or
instead you can list examples of artifacts.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rst
File src/stats/doc/aggregator.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rs...
src/stats/doc/aggregator.rst:58: - A space separated gnuplot data file
Are there any arguments pro and contra space separation compared to, say, tab
separation?

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rs...
src/stats/doc/aggregator.rst:205: - Space separated (the default)
Same comment as above.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rs...
src/stats/doc/aggregator.rst:370: // Enable logging of data for the aggregator.
I would say aggregation instead of logging here.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rs...
src/stats/doc/aggregator.rst:395: // Disable logging of data for the aggregator.
Same comment as above.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/data-collecti...
File src/stats/doc/data-collection-helpers.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/data-collecti...
src/stats/doc/data-collection-helpers.rst:96: A fully worked example (from
``sixth-dcf.cc``) is shown below:
seventh.cc

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/probe.rst
File src/stats/doc/probe.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/probe.rst#new...
src/stats/doc/probe.rst:37: trace source are as follows:
Very good.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/probe.rst#new...
src/stats/doc/probe.rst:51: Stat::put ("my_metric", ID, sample);
Is this our source code/interface? Perhaps we should refer to ns2measure, but
put our source code here.

https://codereview.appspot.com/10974043/diff/3001/src/stats/examples/file-agg...
File src/stats/examples/file-aggregator-example.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/examples/file-agg...
src/stats/examples/file-aggregator-example.cc:54: for (time = -5.0; time <=
+5.0; time += 1.0)
Potential double comparison problem?

https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
File src/stats/model/time-series-adaptor.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
src/stats/model/time-series-adaptor.cc:72: m_output (Simulator::Now
().GetSeconds (), newData);
I assume this will be generalized?

[Tom Henderson, 2013-07-30 21:16:29]

thanks for you review; I will handle all the comments except the last one that I
did not understand.

https://codereview.appspot.com/10974043/diff/3001/.hgignore
File .hgignore (right):

https://codereview.appspot.com/10974043/diff/3001/.hgignore#newcode30
.hgignore:30: ^doc/experiment/source-temp
I need to omit the above two statements for now

https://codereview.appspot.com/10974043/diff/3001/.hgignore#newcode44
.hgignore:44: \.plt$
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Not related to DCF, but I like this very much.

DCF tends to generate output files with these suffixes

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/conf.py
File doc/manual/source/conf.py (right):

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/conf.py#n...
doc/manual/source/conf.py:53: release = 'ns-3.16'
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Should probably be updated.

will fix this nit

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/index.rst
File doc/manual/source/index.rst (right):

https://codereview.appspot.com/10974043/diff/3001/doc/manual/source/index.rst...
doc/manual/source/index.rst:28: data-collection
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Redundant?

at the moment, no-- separate from statistics documentation (which will be
revamped in the future)

https://codereview.appspot.com/10974043/diff/3001/examples/tutorial/seventh.cc
File examples/tutorial/seventh.cc (right):

https://codereview.appspot.com/10974043/diff/3001/examples/tutorial/seventh.c...
examples/tutorial/seventh.cc:264: Simulator::Destroy ();
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Upon running this, I get .plt file with:
> 
> set terminal png
> set output "gnuplot-helper-plot.png"
> set title "Gnuplot Helper Plot"
> set xlabel "X Values"
> set ylabel "Y Values"
> 
> set datafile missing "-nan"
> 

This is a bug that Mitch is looking into (I may have introduced with some late
edits).  this will be fixed
> and empty .dat file. Is this intended?

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
File src/network/utils/packet-data-calculators.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
src/network/utils/packet-data-calculators.cc:37: NS_LOG_FUNCTION_NOARGS ();
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Should be NS_LOG_FUNCTION (this);

This is an existing file; not sure why it shows up as a new file here.

Yes, this file needs logging fixes.

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
File src/network/utils/packet-data-calculators.h (right):

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
src/network/utils/packet-data-calculators.h:40: void PacketUpdate (std::string
path, Ptr<const Packet> packet);
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Missing documentation

This is an existing file; not sure why it is being treated as a new file.

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/adaptor.rst
File src/stats/doc/adaptor.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/adaptor.rst#n...
src/stats/doc/adaptor.rst:16: part, to generate artifacts with their
simulation's results.
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Artifacts could also mean errors, perhaps a better word could be used here, or
> instead you can list examples of artifacts.

ok, 'results' may be better

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rst
File src/stats/doc/aggregator.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/aggregator.rs...
src/stats/doc/aggregator.rst:58: - A space separated gnuplot data file
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Are there any arguments pro and contra space separation compared to, say, tab
> separation?

I don't know offhand, seems to be a gnuplot question

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/data-collecti...
File src/stats/doc/data-collection-helpers.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/data-collecti...
src/stats/doc/data-collection-helpers.rst:96: A fully worked example (from
``sixth-dcf.cc``) is shown below:
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> seventh.cc

correct, thanks

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/probe.rst
File src/stats/doc/probe.rst (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/doc/probe.rst#new...
src/stats/doc/probe.rst:51: Stat::put ("my_metric", ID, sample);
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Is this our source code/interface? Perhaps we should refer to ns2measure, but
> put our source code here.

OK, will consider this.  The code snippet is from ns2measure

https://codereview.appspot.com/10974043/diff/3001/src/stats/examples/file-agg...
File src/stats/examples/file-aggregator-example.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/examples/file-agg...
src/stats/examples/file-aggregator-example.cc:54: for (time = -5.0; time <=
+5.0; time += 1.0)
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> Potential double comparison problem?

OK

https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
File src/stats/model/time-series-adaptor.cc (right):

https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
src/stats/model/time-series-adaptor.cc:72: m_output (Simulator::Now
().GetSeconds (), newData);
On 2013/07/30 19:15:32, Vedran Miletić wrote:
> I assume this will be generalized?

do not know what you mean

[Tom Henderson, 2013-07-30 21:31:34]

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
File src/network/utils/packet-data-calculators.h (right):

https://codereview.appspot.com/10974043/diff/3001/src/network/utils/packet-da...
src/network/utils/packet-data-calculators.h:40: void PacketUpdate (std::string
path, Ptr<const Packet> packet);
On 2013/07/30 21:16:29, Tom Henderson wrote:
> On 2013/07/30 19:15:32, Vedran Miletić wrote:
> > Missing documentation
> 
> This is an existing file; not sure why it is being treated as a new file.

I think I understand now-- it had to get moved out of stats with the new module
reorg.  This is an artifact of how the code review was generated-- it will be
outside of the final patch.

[Vedran Miletić, 2013-07-31 11:35:26]

On 2013/07/30 21:16:29, Tom Henderson wrote:
>
https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
> File src/stats/model/time-series-adaptor.cc (right):
> 
>
https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
> src/stats/model/time-series-adaptor.cc:72: m_output (Simulator::Now
> ().GetSeconds (), newData);
> On 2013/07/30 19:15:32, Vedran Miletić wrote:
> > I assume this will be generalized?
> 
> do not know what you mean

Documentation mentions configurable time resolution, so the question is along
the lines "will this be generalized eventually to allow configuring other time
resolution than seconds"?

Regards,
Vedran

[Tom Henderson, 2013-08-05 04:18:48]

On 2013/07/31 11:35:26, Vedran Miletić wrote:
> On 2013/07/30 21:16:29, Tom Henderson wrote:
> >
>
https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
> > File src/stats/model/time-series-adaptor.cc (right):
> > 
> >
>
https://codereview.appspot.com/10974043/diff/3001/src/stats/model/time-series...
> > src/stats/model/time-series-adaptor.cc:72: m_output (Simulator::Now
> > ().GetSeconds (), newData);
> > On 2013/07/30 19:15:32, Vedran Miletić wrote:
> > > I assume this will be generalized?
> > 
> > do not know what you mean
> 
> Documentation mentions configurable time resolution, so the question is along
> the lines "will this be generalized eventually to allow configuring other time
> resolution than seconds"?

Yes, I forgot that we hadn't completed that aspect-- I fixed the documentation
to say that it is not quite done yet.



> 
> Regards,
> Vedran