Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(61)

Issue 14677043: UAN-WOSS framework

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by Fedwar
Modified:
1 year, 10 months ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com, ns-3-reviews_googlegroup.com
Visibility:
Public.

Description

UAN-WOSS framework

Patch Set 1 #

Total comments: 1

Patch Set 2 : UAN-WOSS integration framework review #

Patch Set 3 : UAN-WOSS fixed all warnings #

Patch Set 4 : WOSS Integration Framework patch rework #

Total comments: 57

Patch Set 5 : all comments have been addressed. #

Patch Set 6 : FH-BFSK interf model revised, thanks to Randall Plate #

Patch Set 7 : random generator and UanPdp generation revised #

Patch Set 8 : Bug fixes thanks to Randall and Raul, new features added as per WOSS 1.5.0 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6079 lines, -17 lines) Patch
M doc/main.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M doc/models/Makefile View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M doc/models/source/index.rst View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/mobility/model/waypoint-mobility-model.h View 1 2 3 4 5 6 2 chunks +2 lines, -1 line 0 comments Download
M src/mobility/model/waypoint-mobility-model.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M src/uan/model/uan-channel.h View 1 2 3 4 3 chunks +5 lines, -6 lines 0 comments Download
M src/uan/model/uan-net-device.h View 1 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M src/uan/model/uan-net-device.cc View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download
M src/uan/model/uan-phy-gen.h View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M src/uan/model/uan-phy-gen.cc View 1 2 3 4 5 2 chunks +13 lines, -7 lines 0 comments Download
M src/uan/model/uan-prop-model.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M src/uan/model/uan-prop-model.cc View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
A src/woss/doc/woss.h View 1 2 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
A src/woss/doc/woss.rst View 1 2 3 1 chunk +177 lines, -0 lines 0 comments Download
A src/woss/examples/woss-aloha-example.h View 1 2 3 4 1 chunk +99 lines, -0 lines 0 comments Download
A src/woss/examples/woss-aloha-example.cc View 1 2 3 4 5 1 chunk +310 lines, -0 lines 0 comments Download
A src/woss/examples/wscript View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
A src/woss/helper/woss-helper.h View 1 2 3 4 5 6 7 1 chunk +600 lines, -0 lines 0 comments Download
A src/woss/helper/woss-helper.cc View 1 2 3 4 5 6 7 1 chunk +1555 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-location.h View 1 2 3 4 1 chunk +74 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-location.cc View 1 2 3 4 1 chunk +67 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-random-generator.h View 1 2 3 4 5 6 1 chunk +137 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-random-generator.cc View 1 2 3 4 5 6 1 chunk +103 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-time-reference.h View 1 2 3 4 1 chunk +69 lines, -0 lines 0 comments Download
A src/woss/model/definitions/woss-time-reference.cc View 1 2 3 4 1 chunk +48 lines, -0 lines 0 comments Download
A src/woss/model/woss-channel.h View 1 2 3 4 1 chunk +78 lines, -0 lines 0 comments Download
A src/woss/model/woss-channel.cc View 1 2 3 4 5 6 1 chunk +238 lines, -0 lines 0 comments Download
A src/woss/model/woss-position-allocator.h View 1 2 3 4 1 chunk +424 lines, -0 lines 0 comments Download
A src/woss/model/woss-position-allocator.cc View 1 2 3 4 1 chunk +640 lines, -0 lines 0 comments Download
A src/woss/model/woss-prop-model.h View 1 2 3 4 5 6 7 1 chunk +155 lines, -0 lines 0 comments Download
A src/woss/model/woss-prop-model.cc View 1 2 3 4 5 6 7 1 chunk +324 lines, -0 lines 0 comments Download
A src/woss/model/woss-waypoint-mobility-model.h View 1 2 3 4 1 chunk +71 lines, -0 lines 0 comments Download
A src/woss/model/woss-waypoint-mobility-model.cc View 1 2 3 4 1 chunk +177 lines, -0 lines 0 comments Download
A src/woss/test/examples-to-run.py View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download
A src/woss/test/woss-test.cc View 1 2 3 4 5 6 1 chunk +441 lines, -0 lines 0 comments Download
A src/woss/wscript View 1 2 3 4 5 1 chunk +165 lines, -0 lines 0 comments Download

Messages

Total messages: 10
Tom Henderson
I had a scan of this code, and at first glance, the code look reasonable ...
4 years, 8 months ago (2014-02-10 14:26:55 UTC) #1
Fedwar
Hi Tom, first of all: thanks for the review! I'll reply inline: *What this would ...
4 years, 8 months ago (2014-02-10 22:44:09 UTC) #2
Fedwar
Hi Tom, any update? thanks Federico
4 years, 4 months ago (2014-06-09 13:15:04 UTC) #3
Tom Henderson
I think the changes to the src/uan module are acceptable. Please see my comments on ...
2 years, 5 months ago (2016-04-25 17:45:07 UTC) #4
Fedwar
Hi Tom, I've replied to almost all comments. I will provide a new patch once ...
2 years, 5 months ago (2016-04-30 21:18:09 UTC) #5
Fedwar
https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helper.cc File src/woss/helper/woss-helper.cc (right): https://codereview.appspot.com/14677043/diff/43001/src/woss/helper/woss-helper.cc#newcode117 src/woss/helper/woss-helper.cc:117: // << c9 << " " << h2 << ...
2 years, 5 months ago (2016-05-01 09:38:39 UTC) #6
Tom Henderson
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.cc#newcode296 src/uan/model/uan-mac-cw.cc:296: // not used. On 2016/04/30 21:18:08, fedwar wrote: > ...
2 years, 5 months ago (2016-05-07 14:57:52 UTC) #7
Fedwar
https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-model.h File src/uan/model/uan-prop-model.h (right): https://codereview.appspot.com/14677043/diff/43001/src/uan/model/uan-prop-model.h#newcode238 src/uan/model/uan-prop-model.h:238: * Creates a normalized PDP to its non coherent ...
2 years, 5 months ago (2016-05-08 16:19:48 UTC) #8
Fedwar
Hi Tom, I've added some comments for you to check. please check them all and ...
2 years, 5 months ago (2016-05-12 12:57:28 UTC) #9
Tom Henderson
2 years, 5 months ago (2016-05-12 16:26:38 UTC) #10
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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b