UAN-WOSS framework

UAN-WOSS framework

[Tom Henderson, 2014-02-10 14:26:55]

I had a scan of this code, and at first glance, the code look reasonable and
nearly ready, but we need to address integration, documentation, example code,
and test code.

Let's first discuss integration issues.  This module requires some installation
of other components (woss, netcdf).  It seems like it may be appropriate to try
to integrate this module with the bake build system for ns-3, allowing bake to
handle the prerequisite installation and checks, then allowing uan-woss module
to be downloaded and installed optionally.

What this would mean is that we would review and merge the changes to the
existing ns-3, and then we would arrange it such that the bake tool would
separately download the 'uan-woss' module and its prerequisites and install it
alongside the 'uan' module.  Would this be an acceptable integration path for
you?

Second, please create some user documentation on what this is modeling and what
is the expected user API that users will need to typically handle.  There are a
lot of attributes being set by the helper Init method; are these documented
elsewhere?  Which attributes are the key ones, and which ones are rarely
touched?  I realize that this code is a wrapper to the external WOSS framework,
so it may be sufficient (if documentation exists online for WOSS) to have the
ns-3 documentation be mainly pointers to the WOSS documentation.

The example does not seem to do much beyond the setup phase; is a richer example
possible?  Possibly something that outputs data for a WOSS-relevant scenario
that can be used for plotting?

The test code seems to just check for a few basic unit tests (are channels set
up symmetrically, do two packets collide when expected).  Again, is there any
canonical test of WOSS (e.g. checking figure points on a curve are within
tolerance) that can somehow be run here?  Does WOSS itself have comprehensive
testing and we only need here to test the integration aspects?  

Please let me know how you perceive that overall testing of WOSS is accomplished
(ns-3 tests + WOSS tests, or just ns-3 tests), and what you perceive to be the
key aspects of the model that should be tested here, and we can discuss next
steps.

https://codereview.appspot.com/14677043/diff/1/src/uan/test/uan-woss-test.cc
File src/uan/test/uan-woss-test.cc (right):

https://codereview.appspot.com/14677043/diff/1/src/uan/test/uan-woss-test.cc#...
src/uan/test/uan-woss-test.cc:92: m_wossHelper->SetAttribute ("ResDbFileName",
StringValue ("test.dat"));
This is something that we need to address in the integration (explicit file
paths).

Is there a reason that all of these attributes have to be set?  Are they all
non-default values?  If they are just resetting to the default, can we eliminate
the SetAttribute here?

[Fedwar, 2014-02-10 22:44:09]

Hi Tom,
first of all: thanks for the review!

I'll reply inline:





*What this would mean is that we would review and merge the changes to the
existing ns-3, and then we would arrange it such that the bake toolwould
separately download the 'uan-woss' module and its prerequisites and install
it alongside the 'uan' module.  Would this be an acceptableintegration path
for you?*
Yes it would be OK :)
Unfortunately I don't know how to use the bake tool, therefore I would need
an how-to in order to write the configuration (if needed).









*second, please create some user documentation on what this is modeling and
what is the expected user API that users will need to typicallyhandle.
 There are a lot of attributes being set by the helper Init method; are
these documented elsewhere?  Which attributes are the keyones, and which
ones are rarely touched?  I realize that this code is a wrapper to the
external WOSS framework, so it may be sufficient (ifdocumentation exists
online for WOSS) to have the ns-3 documentation be mainly pointers to the
WOSS documentation.*

 Ok no problem, I will write some detailed documentation, could you point
me out the format (doxygen? html page? template?)
The WOSS in NS3 mainly boils down to a new channel, phy and propagation
models which are an extension of the UAN's classes, so no new API's here
for the typical UAN user.
The wrapper classes should be considered "core" and should not be used by
the common user.
The helper class has many attributes but alas they are needed since the
WOSS framework is rather complex!
They are used to set up the environment data, the channel simulator params
etc...
However I agree that this needs an expanded description.

Btw, WOSS documentation is online and is located here
http://telecom.dei.unipd.it/ns/woss/doxygen/




*The example does not seem to do much beyond the setup phase; is a richer
example possible?  Possibly something that outputs data for aWOSS-relevant
scenario that can be used for plotting?*
WOSS is just a mean to plug a more realistic channel simulator into UAN in
order to get more realistic results with the current network devices. I
usually don't use WOSS to plot the channel, since this is not the main
objective. (but it could be added later if needed). A richer example would
only involve more nodes, with different devices, but I think that it won't
add that much. Do you agree?






*The test code seems to just check for a few basic unit tests (are channels
set up symmetrically, do two packets collide when expected).Again, is there
any canonical test of WOSS (e.g. checking figure points on a curve are
within tolerance) that can somehow be run here?  DoesWOSS itself have
comprehensive testing and we only need here to test the integration
aspects?*
WOSS unfortunately has no built-in test unit, but it was tested and
validated through the years of development and protocol testing.
WOSS currently is based on BELLHOP channel simulator, which is currently
accepted by the underwater community (http://oalib.hlsresearch.com/Rays/).




*Please let me know how you perceive that overall testing of WOSS is
accomplished (ns-3 tests + WOSS tests, or just ns-3 tests), and what
youperceive to be the key aspects of the model that should be tested here,
and we can discuss next steps.*
NS-3 tests would do.
Additional key tests would be:
- correct environmental data retrieval (custom and from databases (if
installed!) )
- correct channel simulation (partially done)




*Is there a reason that all of these attributes have to be set?  Are they
all non-default values?  If they are just resetting to the default, canwe
eliminate the SetAttribute here?*
Most of them are default and can be removed.

Thanks again,

Federico

[Fedwar, 2014-06-09 13:15:04]

Hi Tom,
any update?

thanks

Federico

[Tom Henderson, 2016-04-25 17:45:07]

I think the changes to the src/uan module are acceptable.

Please see my comments on some small improvements to the src/woss module.  In
particular, the wrappers for random variables and time, and the issue with
respect to hardcoded directory paths.

In general, I envision that for ns-3.26, we could provide this as follows.

1) merge these base uan module changes to ns-3-dev

2) provide the 'woss' module as a contributed code module under src/contrib/,
once the bake/Waf support is added for this

3) provide support for users to get and build netcdf, acoustics toolbox, etc.
through bake system

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.cc
File src/uan/model/uan-mac-cw.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.c...
src/uan/model/uan-mac-cw.cc:296: // not used.
Please explain why this is added but not used.

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
File src/uan/model/uan-prop-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
src/uan/model/uan-prop-model.h:238: * Creates a normalized PDP to its non
coherent sum
this documentation is too brief; is there a reference to 'non coherent sum'
somewhere that you could include?

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.cc:80: wossHelper->SetAttribute
("ResDbFilePath", StringValue ("/dev/shm/ns3_woss_aloha_example/"));
this is a bit clumsy for end users to hardcode the database file path.  I think
that what is needed here is a relative path to where we expect this database to
be once the eventual installation is settled.

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.h:20: 

More documentation about this example is needed, provided as a comment here in
this file.

1) What is the point of the example?  What does this example intend to do?

2) What user-observable output (files left behind, messages to the terminal)
does the example produce?

3) what are the important/interesting parameters to vary?  e.g. from this list:

+  cmd.AddValue ("TotalRate", "Total channel capacity", exp.m_totalRate);
+  cmd.AddValue ("PosAllocSelector", "Position Allocator Selector: 0 list, 1
grid, 2 random rectangle, 3 random disc, 4 uniform disc",
exp.m_posAllocSelector);
+  cmd.AddValue ("UseMultithread", "flag to set the WOSS multithread option",
exp.m_useMultithread);
+  cmd.AddValue ("UseTimeEvolution", "flag to set the WOSS time evolution
option", exp.m_useTimeEvolution);
+  cmd.AddValue ("NumberNodes", "Number of nodes", exp.m_numNodes);
+  cmd.AddValue ("PktSize", "Packet size in bytes", exp.m_pktSize);
+  cmd.AddValue ("SimTime", "Simulation time per trial", exp.m_simTime);

4) what should the user be able to observe (confirm) if he or she runs the
example in a particular way such as with no command line arguments?

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:52: #define WH_WORK_PATH_DEFAULT "/dev/shm/woss"
see my comment elsewhere about how we need to avoid hardcoded absolute path

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:117: //                   << c9 << " " << h2 << "
" << c10 << " " << m2 << " " << c11 << " " << s2);
probably should remove the above

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:253: //   NS_LOG_FUNCTION(this << " step: " <<
step++);
delete these commented out NS_LOG_FUNCTION.  There should be one
NS_LOG_FUNCTION (this);
at the top of this method; if you want to add further debugging logs within the
method, use NS_LOG_DEBUG

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:344: {
NS_LOG_FUNCTION (this << wossPropModel);

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:552: {
NS_LOG_FUNCTION (this << ptr);

please audit your code to add these log statements to each non-trivial method
(e.g. I would not add to operators or simple getters but to any method that you
anticipate a user might want to observe when trying to debug program function
call flow).

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:692: {
Indentation.

I suggest that you use the 'check-style.py' program (requires that you install
'uncrustify' on your system) on each of your new source files.  Run it like this
for each file:

utils/check-style.py -i -l 3 -f /path/to/filename

e.g.

cd ns-3-dev
utils/check-style.py -i -l 3 -f src/woss/helper/woss-helper.cc

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.h:38: * woss::RandomGenerator
wraps the NS3 random generator class into the WOSS framework
this is not very clear to me.  I would assume that you want to use the
implementation of ns-3 RandomVariableStream to support the API of
woss::RandomGenerator.  If so, it is not clear to me what the parameter 'seed'
is controlling.

Also, what should be the semantics of the clone() and copy operations?  What is
the user-perceived behavior if I have a generator 'A' and I clone it to provide
'B' = A->clone(); ?  Is B a new generator (drawing from an independent
substream) or drawing from same stream as A?  I believe it should be independent
but would like for you to clarify this.

p.s. doxygen is mostly missing in this class.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-time-reference.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-time-reference.h:39: * and it is used by the
framework as a simulation time reference
please clarify, does it return the ns-3 Time as a 'double' type with units of
seconds?

What does 'clone()' do?  Please clarify.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:83: */
it is probably cleaner to implement this as a boolean flag that is initialized
to false and set to true within here.  I just wonder about setting it to a
notionally illegal value; might it be used before you have a chance to set it to
a positive time?

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:111: if ( now > m_current.time )
// Won't ever be less, but may be equal
should you check this with an assert?  e.g.

NS_ASSERT (now >= m_current.time);

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.h:55: virtual
~WossWaypointMobilityModel (); //!< Default destructor
missing Update () declaration

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc
File src/woss/test/woss-test.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc...
src/woss/test/woss-test.cc:20: 
please add some comments about what specifically is being tested in this test
program.

[Fedwar, 2016-04-30 21:18:09]

Hi Tom,
I've replied to almost all comments.
I will provide a new patch once we have clarified all open points.
thanks
Federico

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.cc
File src/uan/model/uan-mac-cw.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.c...
src/uan/model/uan-mac-cw.cc:296: // not used.
On 2016/04/25 17:45:06, Tom Henderson wrote:
> Please explain why this is added but not used.

UanPhyListener is parent and this function is pure virtual so this function need
to be impelemented.
However the feature is not yet completed:
- the mac should not rely on an internal timer to deduce that the lower phy has
completed its transmission, but it should rely on a mandatory event from the phy
layer itself.
furthemore the phy should also notify the MAC of tx end due to packet dropped
(i.e. no more energy).

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-phy.h
File src/uan/model/uan-phy.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-phy.h#ne...
src/uan/model/uan-phy.h:171: virtual void NotifyTxEnd (void) = 0;
This function it's part of a interface redesign between PHY and MAC. currently
there is no mandatory tx end event from phy to MAC.
this is awkward, since it forces the MAC to use an internal timer to mimic the
EndTx of the lower phy.
The correct solution would be to ensure that the PHY would always notify the MAC
in case of tx completed or dropped due to energy drained.

I think that this feature should be moved to a separate bugzilla ticket and this
file should be reverted to its previous version

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
File src/uan/model/uan-prop-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
src/uan/model/uan-prop-model.h:238: * Creates a normalized PDP to its non
coherent sum
On 2016/04/25 17:45:06, Tom Henderson wrote:
> this documentation is too brief; is there a reference to 'non coherent sum'
> somewhere that you could include?

No coherent sum means the complex sum of complex number disregarding their
phases (i.e. sum of their absolute values) whilee the non coherent sum is the
sum of its complex values.

as described in "J. F. Bensen et al., Computational Ocean Acoustics".

this is already used and described in current UAN model check 
SumTapsNc

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.cc:80: wossHelper->SetAttribute
("ResDbFilePath", StringValue ("/dev/shm/ns3_woss_aloha_example/"));
On 2016/04/25 17:45:06, Tom Henderson wrote:
> this is a bit clumsy for end users to hardcode the database file path.  I
think
> that what is needed here is a relative path to where we expect this database
to
> be once the eventual installation is settled.

This is not input database path, but the path to the output result databases.
we are simply saving the output database in this directory for the user to
check.
if the user runs the example a second time the WOSS libary would take its
simulated channel from this db instead of re-running the simulation.

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.h:20: 
On 2016/04/25 17:45:06, Tom Henderson wrote:
> 
> More documentation about this example is needed, provided as a comment here in
> this file.
> 
> 1) What is the point of the example?  What does this example intend to do?
> 
> 2) What user-observable output (files left behind, messages to the terminal)
> does the example produce?
> 
> 3) what are the important/interesting parameters to vary?  e.g. from this
list:
> 
> +  cmd.AddValue ("TotalRate", "Total channel capacity", exp.m_totalRate);
> +  cmd.AddValue ("PosAllocSelector", "Position Allocator Selector: 0 list, 1
> grid, 2 random rectangle, 3 random disc, 4 uniform disc",
> exp.m_posAllocSelector);
> +  cmd.AddValue ("UseMultithread", "flag to set the WOSS multithread option",
> exp.m_useMultithread);
> +  cmd.AddValue ("UseTimeEvolution", "flag to set the WOSS time evolution
> option", exp.m_useTimeEvolution);
> +  cmd.AddValue ("NumberNodes", "Number of nodes", exp.m_numNodes);
> +  cmd.AddValue ("PktSize", "Packet size in bytes", exp.m_pktSize);
> +  cmd.AddValue ("SimTime", "Simulation time per trial", exp.m_simTime);
> 
> 4) what should the user be able to observe (confirm) if he or she runs the
> example in a particular way such as with no command line arguments?

Acknowledged.

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:52: #define WH_WORK_PATH_DEFAULT "/dev/shm/woss"
On 2016/04/25 17:45:06, Tom Henderson wrote:
> see my comment elsewhere about how we need to avoid hardcoded absolute path

this is directory that will be used by the woss library in order to save the
temporary files created by bellhop.
please provide example of relative path in order to save them /dev/shm/

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:253: //   NS_LOG_FUNCTION(this << " step: " <<
step++);
On 2016/04/25 17:45:06, Tom Henderson wrote:
> delete these commented out NS_LOG_FUNCTION.  There should be one
> NS_LOG_FUNCTION (this);
> at the top of this method; if you want to add further debugging logs within
the
> method, use NS_LOG_DEBUG

OK

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:344: {
On 2016/04/25 17:45:06, Tom Henderson wrote:
> NS_LOG_FUNCTION (this << wossPropModel);

OK

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:552: {
On 2016/04/25 17:45:06, Tom Henderson wrote:
> NS_LOG_FUNCTION (this << ptr);
> 
> please audit your code to add these log statements to each non-trivial method
> (e.g. I would not add to operators or simple getters but to any method that
you
> anticipate a user might want to observe when trying to debug program function
> call flow).

OK

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:692: {
On 2016/04/25 17:45:06, Tom Henderson wrote:
> Indentation.
> 
> I suggest that you use the 'check-style.py' program (requires that you install
> 'uncrustify' on your system) on each of your new source files.  Run it like
this
> for each file:
> 
> utils/check-style.py -i -l 3 -f /path/to/filename
> 
> e.g.
> 
> cd ns-3-dev
> utils/check-style.py -i -l 3 -f src/woss/helper/woss-helper.cc

Acknowledged.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.h:38: * woss::RandomGenerator
wraps the NS3 random generator class into the WOSS framework
On 2016/04/25 17:45:06, Tom Henderson wrote:
> this is not very clear to me.  I would assume that you want to use the
> implementation of ns-3 RandomVariableStream to support the API of
> woss::RandomGenerator.  If so, it is not clear to me what the parameter 'seed'
> is controlling.
> 
> Also, what should be the semantics of the clone() and copy operations?  What
is
> the user-perceived behavior if I have a generator 'A' and I clone it to
provide
> 'B' = A->clone(); ?  Is B a new generator (drawing from an independent
> substream) or drawing from same stream as A?  I believe it should be
independent
> but would like for you to clarify this.
> 
> p.s. doxygen is mostly missing in this class.

seed is a model independent way to change the substream (or whatever it is
called in each model).
B = A->clone() is a perfect copy of A, since it is called via copy constructor
clone() { return new A(*this); }
clone is used as a virtual factory method used by every woss definition class.
how to create an independent random variable inside woss

B=A->create(seed/substream)

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-time-reference.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-time-reference.h:39: * and it is used by the
framework as a simulation time reference
On 2016/04/25 17:45:06, Tom Henderson wrote:
> please clarify, does it return the ns-3 Time as a 'double' type with units of
> seconds?
> 
> What does 'clone()' do?  Please clarify.

correct double as unit of time.

clone is abstract and it is a virtual factory method that must be implemented by
all definition classes.
WOSS let the user re-implement its definition classes (i.e. its fundamental
classes). In order to manipulate them it has to rely on virtual factory
functions (when and if needed!)

clone in this case returns a new WossTimeReference object created by copy
constructor.

this class simply calls the static method of the NS3 Simulator class

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:83: */
On 2016/04/25 17:45:07, Tom Henderson wrote:
> it is probably cleaner to implement this as a boolean flag that is initialized
> to false and set to true within here.  I just wonder about setting it to a
> notionally illegal value; might it be used before you have a chance to set it
to
> a positive time?

This part of the code is a copy paste of the original Update() const function of
its parent class which is already in NS3 mainline

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.h:55: virtual
~WossWaypointMobilityModel (); //!< Default destructor
On 2016/04/25 17:45:07, Tom Henderson wrote:
> missing Update () declaration

Acknowledged.

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc
File src/woss/test/woss-test.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc...
src/woss/test/woss-test.cc:20: 
On 2016/04/25 17:45:07, Tom Henderson wrote:
> please add some comments about what specifically is being tested in this test
> program.

Acknowledged.

[Fedwar, 2016-05-01 09:38:39]

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:117: //                   << c9 << " " << h2 << "
" << c10 << " " << m2 << " " << c11 << " " << s2);
On 2016/04/25 17:45:06, Tom Henderson wrote:
> probably should remove the above

Acknowledged.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.h:38: * woss::RandomGenerator
wraps the NS3 random generator class into the WOSS framework
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > this is not very clear to me.  I would assume that you want to use the
> > implementation of ns-3 RandomVariableStream to support the API of
> > woss::RandomGenerator.  If so, it is not clear to me what the parameter
'seed'
> > is controlling.
> > 
> > Also, what should be the semantics of the clone() and copy operations?  What
> is
> > the user-perceived behavior if I have a generator 'A' and I clone it to
> provide
> > 'B' = A->clone(); ?  Is B a new generator (drawing from an independent
> > substream) or drawing from same stream as A?  I believe it should be
> independent
> > but would like for you to clarify this.
> > 
> > p.s. doxygen is mostly missing in this class.
> 
> seed is a model independent way to change the substream (or whatever it is
> called in each model).
> B = A->clone() is a perfect copy of A, since it is called via copy constructor
> clone() { return new A(*this); }
> clone is used as a virtual factory method used by every woss definition class.
> how to create an independent random variable inside woss
> 
> B=A->create(seed/substream)

the implementation is mandatory since the method is abstract in the parent class

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-time-reference.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-time-reference.h:39: * and it is used by the
framework as a simulation time reference
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > please clarify, does it return the ns-3 Time as a 'double' type with units
of
> > seconds?
> > 
> > What does 'clone()' do?  Please clarify.
> 
> correct double as unit of time.
> 
> clone is abstract and it is a virtual factory method that must be implemented
by
> all definition classes.
> WOSS let the user re-implement its definition classes (i.e. its fundamental
> classes). In order to manipulate them it has to rely on virtual factory
> functions (when and if needed!)
> 
> clone in this case returns a new WossTimeReference object created by copy
> constructor.
> 
> this class simply calls the static method of the NS3 Simulator class
>  

the method must be implemented since it is abstract in it's parent woss class

[Tom Henderson, 2016-05-07 14:57:52]

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.cc
File src/uan/model/uan-mac-cw.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-mac-cw.c...
src/uan/model/uan-mac-cw.cc:296: // not used.
On 2016/04/30 21:18:08, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > Please explain why this is added but not used.
> 
> UanPhyListener is parent and this function is pure virtual so this function
need
> to be impelemented.
> However the feature is not yet completed:
> - the mac should not rely on an internal timer to deduce that the lower phy
has
> completed its transmission, but it should rely on a mandatory event from the
phy
> layer itself.
> furthemore the phy should also notify the MAC of tx end due to packet dropped
> (i.e. no more energy). 


OK, can you add the above explanation in comments to this method, for future
readers?

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-phy.h
File src/uan/model/uan-phy.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-phy.h#ne...
src/uan/model/uan-phy.h:171: virtual void NotifyTxEnd (void) = 0;
On 2016/04/30 21:18:09, fedwar wrote:
> This function it's part of a interface redesign between PHY and MAC. currently
> there is no mandatory tx end event from phy to MAC.
> this is awkward, since it forces the MAC to use an internal timer to mimic the
> EndTx of the lower phy.
> The correct solution would be to ensure that the PHY would always notify the
MAC
> in case of tx completed or dropped due to energy drained.
> 
> I think that this feature should be moved to a separate bugzilla ticket and
this
> file should be reverted to its previous version

OK

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
File src/uan/model/uan-prop-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
src/uan/model/uan-prop-model.h:238: * Creates a normalized PDP to its non
coherent sum
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > this documentation is too brief; is there a reference to 'non coherent sum'
> > somewhere that you could include?
> 
> No coherent sum means the complex sum of complex number disregarding their
> phases (i.e. sum of their absolute values) whilee the non coherent sum is the
> sum of its complex values.
> 
> as described in "J. F. Bensen et al., Computational Ocean Acoustics".
> 
> this is already used and described in current UAN model check 
> SumTapsNc

There is a Doxygen 'see also' tag that can be used to cross-reference:
\see SumTapsNc

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.cc:80: wossHelper->SetAttribute
("ResDbFilePath", StringValue ("/dev/shm/ns3_woss_aloha_example/"));
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > this is a bit clumsy for end users to hardcode the database file path.  I
> think
> > that what is needed here is a relative path to where we expect this database
> to
> > be once the eventual installation is settled.
> 
> This is not input database path, but the path to the output result databases.
> we are simply saving the output database in this directory for the user to
> check.
> if the user runs the example a second time the WOSS libary would take its
> simulated channel from this db instead of re-running the simulation.

OK, can we guarantee that /dev/shm will be present (outside of Linux)?  Would it
be better here to point the example to use current working directory, and leave
a comment that users can direct the file to /dev/shm if they want to store in
RAM?

If I understand correctly, this attribute must be set to an absolute file system
path, so I suggest perhaps:

- wossHelper->SetAttribute ("ResDbFilePath", StringValue ("/dev/shm/ns3_woss_alo
ha_example/"));
+ // This saves the output database in this directory for future reuse
+ // Set to absolute file system path; may use /dev/shm if your system supports
it
+ wossHelper->SetAttribute ("ResDbFilePath", StringValue ("/tmp/ns3_woss_alo
ha_example/"));

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:52: #define WH_WORK_PATH_DEFAULT "/dev/shm/woss"
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > see my comment elsewhere about how we need to avoid hardcoded absolute path
> 
> this is directory that will be used by the woss library in order to save the
> temporary files created by bellhop.
> please provide example of relative path in order to save them /dev/shm/

so are you saying that WOSS requires absolute path?  If so, I recommend to use
/tmp/ just because it is more standardized to be always available than /dev/shm

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.cc:78: return const_cast<
Ptr<UniformRandomVariable> & >(m_RandVar)->GetInteger (WOSS_INTEGER_MIN,
WOSS_INTEGER_MAX);
why doesn't GetInteger() without the const_cast work?

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.h:38: * woss::RandomGenerator
wraps the NS3 random generator class into the WOSS framework
On 2016/05/01 09:38:39, fedwar wrote:

> the implementation is mandatory since the method is abstract in the parent
class

OK, I suggest more documentation on how ns-3 random variables map to Woss.

In ns-3, there is primarily the concept of 'seed' and 'run number'.  'run
number' is what users should be typically changing.  Within the (very large)
pseudo-random sequence generated by a particular run number, there is a concept
of a stream which is provided to each instance of a RandomVariableStream.  The
stream assignment to RandomVariableStream is auto-assigned (static
intitialized).  Because the order of static initialization is not guaranteed,
stream assignments to the corresponding random variables may be perturbed by
small changes to the topology.  This is why the 'AssignStreams' was introduced,
to fix the stream assignment so that any randomness changes to the
RandomVariableStream instances will only occur due to changes to the global seed
or run number.

So, I would suggest that you clarify if all of the above properties hold in this
wrapper class:
- clarify that it wraps an underlying UniformRandomVariable bounded by MIN and
MAX
- clarify whether the 'seed' argument in the constructor has any effect (if so,
what), or is just there to satisfy API constraints of the base class?
- clarify whether create() will create a new underlying UniformRandomVariable
(and hence user may need to call AssignStreams again on it if he or she cares
about stream assignment)?
- clarify whether clone() will essentially create another pointer to the same
underlying UniformRandomVariable stream (e.g. if B is clone of A, B and A both
draw on the same underlying UniformRandomVariable instance)

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-time-reference.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-time-reference.h:39: * and it is used by the
framework as a simulation time reference
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > please clarify, does it return the ns-3 Time as a 'double' type with units
of
> > seconds?
> > 
> > What does 'clone()' do?  Please clarify.
> 
> correct double as unit of time.
> 
> clone is abstract and it is a virtual factory method that must be implemented
by
> all definition classes.
> WOSS let the user re-implement its definition classes (i.e. its fundamental
> classes). In order to manipulate them it has to rely on virtual factory
> functions (when and if needed!)
> 
> clone in this case returns a new WossTimeReference object created by copy
> constructor.
> 
> this class simply calls the static method of the NS3 Simulator class
>  

OK, I suggest to add above comments to the Doxygen.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:83: */
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:07, Tom Henderson wrote:
> > it is probably cleaner to implement this as a boolean flag that is
initialized
> > to false and set to true within here.  I just wonder about setting it to a
> > notionally illegal value; might it be used before you have a chance to set
it
> to
> > a positive time?
> 
> This part of the code is a copy paste of the original Update() const function
of
> its parent class which is already in NS3 mainline

Hmm, maybe it should be fixed there too... I don't understand why Update () is
const (in the parent class).

[Fedwar, 2016-05-08 16:19:48]

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
File src/uan/model/uan-prop-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-mod...
src/uan/model/uan-prop-model.h:238: * Creates a normalized PDP to its non
coherent sum
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/04/30 21:18:09, fedwar wrote:
> > On 2016/04/25 17:45:06, Tom Henderson wrote:
> > > this documentation is too brief; is there a reference to 'non coherent
sum'
> > > somewhere that you could include?
> > 
> > No coherent sum means the complex sum of complex number disregarding their
> > phases (i.e. sum of their absolute values) whilee the non coherent sum is
the
> > sum of its complex values.
> > 
> > as described in "J. F. Bensen et al., Computational Ocean Acoustics".
> > 
> > this is already used and described in current UAN model check 
> > SumTapsNc
> 
> There is a Doxygen 'see also' tag that can be used to cross-reference:
> \see SumTapsNc

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.cc:80: wossHelper->SetAttribute
("ResDbFilePath", StringValue ("/dev/shm/ns3_woss_aloha_example/"));
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/04/30 21:18:09, fedwar wrote:
> > On 2016/04/25 17:45:06, Tom Henderson wrote:
> > > this is a bit clumsy for end users to hardcode the database file path.  I
> > think
> > > that what is needed here is a relative path to where we expect this
database
> > to
> > > be once the eventual installation is settled.
> > 
> > This is not input database path, but the path to the output result
databases.
> > we are simply saving the output database in this directory for the user to
> > check.
> > if the user runs the example a second time the WOSS libary would take its
> > simulated channel from this db instead of re-running the simulation.
> 
> OK, can we guarantee that /dev/shm will be present (outside of Linux)?  Would
it
> be better here to point the example to use current working directory, and
leave
> a comment that users can direct the file to /dev/shm if they want to store in
> RAM?
> 
> If I understand correctly, this attribute must be set to an absolute file
system
> path, so I suggest perhaps:
> 
> - wossHelper->SetAttribute ("ResDbFilePath", StringValue
("/dev/shm/ns3_woss_alo
> ha_example/"));
> + // This saves the output database in this directory for future reuse
> + // Set to absolute file system path; may use /dev/shm if your system
supports
> it
> + wossHelper->SetAttribute ("ResDbFilePath", StringValue ("/tmp/ns3_woss_alo
> ha_example/"));

I will change the path to "./something", please explain where this output would
go.

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
File src/woss/examples/woss-aloha-example.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/examples/woss-alo...
src/woss/examples/woss-aloha-example.h:20: 
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:06, Tom Henderson wrote:
> > 
> > More documentation about this example is needed, provided as a comment here
in
> > this file.
> > 
> > 1) What is the point of the example?  What does this example intend to do?
> > 
> > 2) What user-observable output (files left behind, messages to the terminal)
> > does the example produce?
> > 
> > 3) what are the important/interesting parameters to vary?  e.g. from this
> list:
> > 
> > +  cmd.AddValue ("TotalRate", "Total channel capacity", exp.m_totalRate);
> > +  cmd.AddValue ("PosAllocSelector", "Position Allocator Selector: 0 list, 1
> > grid, 2 random rectangle, 3 random disc, 4 uniform disc",
> > exp.m_posAllocSelector);
> > +  cmd.AddValue ("UseMultithread", "flag to set the WOSS multithread
option",
> > exp.m_useMultithread);
> > +  cmd.AddValue ("UseTimeEvolution", "flag to set the WOSS time evolution
> > option", exp.m_useTimeEvolution);
> > +  cmd.AddValue ("NumberNodes", "Number of nodes", exp.m_numNodes);
> > +  cmd.AddValue ("PktSize", "Packet size in bytes", exp.m_pktSize);
> > +  cmd.AddValue ("SimTime", "Simulation time per trial", exp.m_simTime);
> > 
> > 4) what should the user be able to observe (confirm) if he or she runs the
> > example in a particular way such as with no command line arguments?
> 
> Acknowledged.

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.cc:78: return const_cast<
Ptr<UniformRandomVariable> & >(m_RandVar)->GetInteger (WOSS_INTEGER_MIN,
WOSS_INTEGER_MAX);
On 2016/05/07 14:57:52, Tom Henderson wrote:
> why doesn't GetInteger() without the const_cast work?

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-random-generator.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-random-generator.h:38: * woss::RandomGenerator
wraps the NS3 random generator class into the WOSS framework
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/05/01 09:38:39, fedwar wrote:
> 
> > the implementation is mandatory since the method is abstract in the parent
> class
> 
> OK, I suggest more documentation on how ns-3 random variables map to Woss.
> 
> In ns-3, there is primarily the concept of 'seed' and 'run number'.  'run
> number' is what users should be typically changing.  Within the (very large)
> pseudo-random sequence generated by a particular run number, there is a
concept
> of a stream which is provided to each instance of a RandomVariableStream.  The
> stream assignment to RandomVariableStream is auto-assigned (static
> intitialized).  Because the order of static initialization is not guaranteed,
> stream assignments to the corresponding random variables may be perturbed by
> small changes to the topology.  This is why the 'AssignStreams' was
introduced,
> to fix the stream assignment so that any randomness changes to the
> RandomVariableStream instances will only occur due to changes to the global
seed
> or run number.
> 
> So, I would suggest that you clarify if all of the above properties hold in
this
> wrapper class:
> - clarify that it wraps an underlying UniformRandomVariable bounded by MIN and
> MAX
> - clarify whether the 'seed' argument in the constructor has any effect (if
so,
> what), or is just there to satisfy API constraints of the base class?
> - clarify whether create() will create a new underlying UniformRandomVariable
> (and hence user may need to call AssignStreams again on it if he or she cares
> about stream assignment)?
> - clarify whether clone() will essentially create another pointer to the same
> underlying UniformRandomVariable stream (e.g. if B is clone of A, B and A both
> draw on the same underlying UniformRandomVariable instance)
> 

ok will do.
please also check my private e-mail on this topic

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
File src/woss/model/definitions/woss-time-reference.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/definitions...
src/woss/model/definitions/woss-time-reference.h:39: * and it is used by the
framework as a simulation time reference
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/04/30 21:18:09, fedwar wrote:
> > On 2016/04/25 17:45:06, Tom Henderson wrote:
> > > please clarify, does it return the ns-3 Time as a 'double' type with units
> of
> > > seconds?
> > > 
> > > What does 'clone()' do?  Please clarify.
> > 
> > correct double as unit of time.
> > 
> > clone is abstract and it is a virtual factory method that must be
implemented
> by
> > all definition classes.
> > WOSS let the user re-implement its definition classes (i.e. its fundamental
> > classes). In order to manipulate them it has to rely on virtual factory
> > functions (when and if needed!)
> > 
> > clone in this case returns a new WossTimeReference object created by copy
> > constructor.
> > 
> > this class simply calls the static method of the NS3 Simulator class
> >  
> 
> OK, I suggest to add above comments to the Doxygen.

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:83: */
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/04/30 21:18:09, fedwar wrote:
> > On 2016/04/25 17:45:07, Tom Henderson wrote:
> > > it is probably cleaner to implement this as a boolean flag that is
> initialized
> > > to false and set to true within here.  I just wonder about setting it to a
> > > notionally illegal value; might it be used before you have a chance to set
> it
> > to
> > > a positive time?
> > 
> > This part of the code is a copy paste of the original Update() const
function
> of
> > its parent class which is already in NS3 mainline
> 
> Hmm, maybe it should be fixed there too... I don't understand why Update () is
> const (in the parent class).  

Update is const because it is called by other const function
as GetNextWaypoint () const etc..
which they do call Update() before returning the value, and they are not const
at all.
changing the signature of all these functions would disrupt the functionality of
many models. The same applies to the mentioned flag: I suggest to leave this as
it is and maybe open a new ticket to the mobility mantainer regarding this
discussion, after the WOSS has been properly merged. What do you say?

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.cc:111: if ( now > m_current.time )
// Won't ever be less, but may be equal
On 2016/04/25 17:45:07, Tom Henderson wrote:
> should you check this with an assert?  e.g.
> 
> NS_ASSERT (now >= m_current.time);

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
File src/woss/model/woss-waypoint-mobility-model.h (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/model/woss-waypoi...
src/woss/model/woss-waypoint-mobility-model.h:55: virtual
~WossWaypointMobilityModel (); //!< Default destructor
On 2016/04/25 17:45:07, Tom Henderson wrote:
> missing Update () declaration

actually the declaration is present and it is the private: section

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc
File src/woss/test/woss-test.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc...
src/woss/test/woss-test.cc:20: 
On 2016/04/30 21:18:09, fedwar wrote:
> On 2016/04/25 17:45:07, Tom Henderson wrote:
> > please add some comments about what specifically is being tested in this
test
> > program.
> 
> Acknowledged.

Done.

https://codereview.appspot.com/14677043/diff/43001/src/woss/test/woss-test.cc...
src/woss/test/woss-test.cc:88: m_wossHelper->SetAttribute ("ResDbFilePath",
StringValue ("/dev/shm/ns3_woss_test/"));
changing this path to "./ns3-woss_test/results" leads to the creation of the
following directory "/ns3-dev/ns3-woss_test/results"
is this acceptable?

[Fedwar, 2016-05-12 12:57:28]

Hi Tom,
I've added some comments for you to check.
please check them all and reply me back within this friday night if possible, so
that I can prepare the new merge candidate this week end.

thanks for the help and the comprehension :)

Federico

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:52: #define WH_WORK_PATH_DEFAULT "/dev/shm/woss"
On 2016/05/07 14:57:52, Tom Henderson wrote:
> On 2016/04/30 21:18:09, fedwar wrote:
> > On 2016/04/25 17:45:06, Tom Henderson wrote:
> > > see my comment elsewhere about how we need to avoid hardcoded absolute
path
> > 
> > this is directory that will be used by the woss library in order to save the
> > temporary files created by bellhop.
> > please provide example of relative path in order to save them /dev/shm/
> 
> so are you saying that WOSS requires absolute path?  If so, I recommend to use
> /tmp/ just because it is more standardized to be always available than
/dev/shm

I will change it to a "./something"

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:357: m_defHandler->setRandGenerator
(m_randomGenProto.clone ());

I still have one doubt regarding the NS3 random generator model.

in my woss-random-generator.h file,
I've created the 

virtual int64_t AssignStreams (int64_t stream);

in order to assign the NS3 substream. ==> the seed is not really used by the
WOSS wrapper.

I was relying on the fact that the framework itself would call the AssignStreams
function

but maybe I got the design wrong. Who is gonna call it and when?

Is the example or the test class supposed to call the AssignStreams () or
anything equivalent?

I propose the following:

  m_randomGenProto.AssignStreams (m_wossRandomGenStream); // assign stream
during woss helper initialization
  m_defHandler->setRandGenerator (m_randomGenProto.clone ()); // pass the random
generator to WOSS framework

[Tom Henderson, 2016-05-12 16:26:38]

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
File src/woss/helper/woss-helper.cc (right):

https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helpe...
src/woss/helper/woss-helper.cc:357: m_defHandler->setRandGenerator
(m_randomGenProto.clone ());
On 2016/05/12 12:57:28, fedwar wrote:
> 
> I still have one doubt regarding the NS3 random generator model.
> 
> in my woss-random-generator.h file,
> I've created the 
> 
> virtual int64_t AssignStreams (int64_t stream);
> 
> in order to assign the NS3 substream. ==> the seed is not really used by the
> WOSS wrapper.
> 
> I was relying on the fact that the framework itself would call the
AssignStreams
> function
> 
> but maybe I got the design wrong. Who is gonna call it and when?

Typically it is called by user code, at runtime.

int main (...)
{
  ...
  int64_t streamNumber = 0;
  Ptr<SomeObject> p = CreateObject<SomeObject> ();
  streamNumber += p->AssignStreams (streamNumber);
  ...
}

Here is why you may (or may not) want to do this.  Let's say that I change my
program somewhere else:

int main (...)
{
  ...
  Ptr<SomeOtherObjectUsingRandomVars> pOther = CreateObject<...> ();
  ...
  int64_t streamNumber = 0;
  Ptr<SomeObject> p = CreateObject<SomeObject> ();
  streamNumber += p->AssignStreams (streamNumber);
  ...
}

The act of creating pOther may affect the stream assignment of p if
AssignStreams is not called.  As a result, p will get different values depending
on whether pOther exists in the script or not.  This is independent of whether
the user increments the global random variable run number or seed.

It is not that p gets an invalid stream, just that it may get a _different_
stream as other parts of the simulation are changed.  Whether it does or not is
not controllable by the user (C++ static initialization order issue).

Should you care about this?  It depends.  Let me give a canonical example. 
Suppose that you wish to compare routing performance of AODV vs OLSR on a random
topology.  Your program swaps out AODV for OLSR and reruns the test.  However,
the act of doing so may change the stream assignment of the random variable
assigning random positions of the topology.  So you are not only changing AODV
for OLSR, you are then running on a different topology, which may skew your
results.  In this case, you want to fix your random topology stream assignment
so that if the user chooses AODV, OLSR, or makes other changes, it does not
perturb the stream assignment that is given to the topology/mobility code.  So
you can use AssignStreams() on the mobility/topology objects to fix this.

So, the question you should ask yourself is whether whoever is using the
WossRandomGenerator object will care about this issue and want to fix the stream
to a fixed value.  If so, provide AssignStreams() and have some object call it
during runtime.
   
> 
> Is the example or the test class supposed to call the AssignStreams () or
> anything equivalent?
> 
> I propose the following:
> 
>   m_randomGenProto.AssignStreams (m_wossRandomGenStream); // assign stream
> during woss helper initialization
>   m_defHandler->setRandGenerator (m_randomGenProto.clone ()); // pass the
random
> generator to WOSS framework

Probably should work.  However, keep in mind that m_randomGenProto.clone() will
only generate a copy of the Ptr to the UniformRandomVariable and, as a result,
all clones will reference the same underlying random variable generator.  Do you
care about this?  If you want them to be independent, clone() should do a deep
copy (and you will need to do a bit more work to get AssignStreams() to work out
for you).