[Ns-developers] 802.11 first changesets to comment on.

[Mathieu Lacage]

On Fri, 2008-11-28 at 10:42 +0100, Timo Bingmann wrote:

> Yes, previously it was like taking txPowerDb and adding/subtracting
> the result of GetLoss(a,b). Thus GetLoss never knows the absolute
> power, which it doesn't need to because dB is logarithmic and the two
> propagation models sum up to a multiplicative loss.
> 
> Nakagami however uses the absolute power value as a RV parameter. So I
> need to add the current txPowerDb to the parameter list, and have
> DoGetLoss() return rxPowerDb. Which then can be passed into the next
> propagation model. So the m_next changes from adding up dB values to a
> function composition.
> Old was rxPowerDb = txPowerDb + DoGetLoss1(a,b) + DoGetLoss2(a,b).
> New is rxPowerDb = DoGetLoss2( DoGetLoss1(txPowerDb, a,b), a,b);

I see. I see now that you have changed all existing models to return the
sum of the losses from DoGetLoss. This looks good to me.
> 
> > > - Rewrote samples/main-random-variables to output all implemented
> RVs.
> > 
> > Looks pretty good. Will also require michele weigle approval.
> 
> Problem is that it depends on the Gnuplot extensions.

you need to merge your gnuplot extensions first then :)

> > 1) I liked the ability to not have to allocate objects on the heap:
> is
> > there a specific reason why you made GnuplotDataset derive from
> > RefCountBase ? I am not saying that you should not: I am just trying
> to
> > understand the rationale.
> 
> Those changes are aimed at creating experiment functions like:
> 
> Ptr<GnuplotDataset> RunExperimentToGenerateOneFunctionLine(Ptr<Object>
> param, int a, ...)
> 
> Using std::vector<Data*> inside GnuplotDataset means copying all the
> dataset's values. So I needed to add reference counting to

What is wrong about copying all the values ? I mean, it's not as if you
were doing something which is costly from a CPU perspective (graph
generation is done once at the end of your simulation and it is,
hopefully a much smaller cpu cost than the simulation itself) and it
makes the API vastly simpler because you don't have to deal with
pointers and such. That was the rationale for the previous value-only
semantics.

>  GnuplotDataset. I did one version with internal (hand-coded)
> reference counting, but obviously using ns-3's RefCount or even Object
> is better.
> 
> Same goes then for GnuplotCollection holding a vector of Gnuplot.

Again, would it not be possible to copy all the values instead ?
> 
> > 2) what is the difference between a dataset title and a dataset
> > function ? (i.e., what is the purpose of
> GnuplotDataset::SetFunction).
> > it looks unused.
> 
> The SetFunction is used to plot non-datapoint line. E.g. the RV
> probability density functions in the RV plot.

Ok. I looked a bit at the implementation. It would make sense to split
this into a GnuplotFunctionDataset class which has no Add (...) method
instead of having a single class have different modes.
> 
> > 3) it would be cool to make DetectTerminal and SetTerminal private
> and
> > call them from the Gnuplot constructor.
> 
> SetTerminal() can be used after construction to add more terminal
> options like "pdf enhanced size 5.0,3.0 ...".

Is this really needed ? I understand that you would like to use this
class for everything but adding knobs everywhere is going to make it
very hard to use for the simple things it was meant to do initially.
> 
> > 4) I would tend to avoid the duplicate functionality provided by
> void
> > AddDataset (Ptr<GnuplotDataset> dataset, const std::string& title);
> and
> > just ask the user to set the title on the dataset himself.
> 
> Useful for:
> gnuplot.AddDataset( RunExperimentToGenerateOneFunctionLine(param1),
> "With Param 1" );
> gnuplot.AddDataset( RunExperimentToGenerateOneFunctionLine(param2),
> "With Param 2" );

you can rewrite to:

gnuplot.AddDataset( RunExperimentToGenerateOneFunctionLine(param1, "With
Param 1"));

and get rid of this duplicate functionality.
> 
> > 5) What do you use extra for ?
> 
> Well gnuplot has so many modifiers and text comments, I doubt it would
> be useful to create a c++ representation of each. Thus AddExtra can be
> used to add any of those formatting commands.
> Eg. it is a was of setting "xrange", "xtics", .. etc or in a
> GnuplotDataset context adding "linecolor 3 pointsize 5"...

I would tend to say that you should create a C++ representation of the
ones you use and ignore the others. Otherwise, you should be generating
the gnuplot output file directly by hand. The purpose of the gnuplot.h
header was to try to make it easy to generate a gnuplot-compatible file
for someone who does not know the intimate details of the gnuplot file
format and its myriad of options by just reading the C++ header.
> 
> > 6) GnuplotDataset::AddEmptyLine is totally evil. Can't you figure
> out a
> > way to get rid of it ?
> 
> No, I've created you a plot of the 3d surface without the empty lines.
> It is unreadable. See
> ns-3-main-propagation-loss-withoutEmptyLines.pdf
> at http://idlebox.net/2008/ns-3-wifi/results/

I understand that you need to make the generated gnuplot file contain
some empty lines at strategic locations. My question was more along the
lines of "why can't you automatically generate these empty lines ?". If
the answer is that this would make the class not able to do
_everything_, then, I think that this is ok.
> 
> > 7) GnuplotDataset::Type looks also pretty evil: would it not make
> sense
> > to simply create an unrelated Gnuplot3dDataset class and make the
> > Gnuplot class be able to also handle it ? This might help with (6)
> > above.
> 
> Yes, but it seemed overkill at that moment. However right now I
> believe making 4 classes is better: GnuplotDataset, GnuplotFunction,
> Gnuplot3dDataset and then a abstract class GnuplotLine (or different
> name?).

What is GnuplotLine ?

> > > fading model. Conceptionally it would be better to separate them:
> use
> > > LogDistanceLossModel followed by the Nakagami model using the
> chaining
> > > feature. However this would require me to extend
> LogDistanceLossModel
> > > into the 3-piecewise composition. It introduces more complexity.
> Maybe
> > > just add a new class ThreeLogDistanceLossModel? What do you think?
> > 
> > This, indeed, seems like the right thing to do.
> 
> Yes. If it's ok to change LogDistanceLossModel into one with 3
> distance ranges and different exponents for each.

>From a usability perspective, I would like to keep the current
LogDistanceLossModel simple so, I would tend to introduce the
ThreeLogDistanceLossModel class instead of modifying the existing
LogDistanceLossModel class.

To summarize our discussion:
1) the propagation model changes seem to be on the right track: I would
be fine with merging them with or without the ThreeLogDistanceLossModel
split. The latter is up to you (I would personally split but I am not
going to maintain this code :).

2) we need to figure out what to do with the gnuplot changes: I think
that the crux of the issue is that we need to identify what is the
purpose of this code. Originally, the purpose was to do something which
is really simple, inefficient (from a CPU perspective), and,
potentially, limited to save me and others from the pain of having to
know the details of the gnuplot commands. 

>From this perspective, new functionality can't be too generic and should
try to stay simple (if somewhat limited). I don't know how easy it would
be to add the features you are interested in in a way which is
compatible with these goals but it would be really nice if you could try
to put together a proposal which does not mandate heap-allocated objects
(at the cost of higher cpu/ram usage), which more clearly separates API
for various kinds of datasets (2d/3d functions, 2d datasets, 3d
datasets), and, which does not expose too much of the internals of
gnuplot.

thanks for replying to my comments so quickly,
Mathieu