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

[Timo Bingmann]

On Friday 28 November 2008 11:26:57 Mathieu Lacage wrote:
> On Fri, 2008-11-28 at 10:42 +0100, Timo Bingmann wrote:
> 
> > > > - 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.

True. Well, the plot data of the RVs amounts to 1MB of text. Not much these days.

I added reference counting because code like the following just cries for reference counting:

Gnuplot::~Gnuplot () {
  for (Datasets::const_iterator i = m_datasets.begin (); i != m_datasets.end (); i++) {
      delete *i;
    }
  m_datasets.clear ();
}

Without copy-constructor and assignment-operator the objects aren't copyable by-value.
 
> >  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 ?

Thinking about that at the moment.

> > > 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"));

True.

> 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 ?

The abstract base class. I believe we're thinking in the same direction.

> > > > 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 :).

I will split them up. ThreeLogDistancePropagationLossModel and NakagamiPropagationLossModel it shall be.

> 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.

Ok, from my perspective it enables very elegant runs: "./experiment | gnuplot"
And no extra gnuplot files lying around. Btw: "./waf --run experiment | gnuplot" doesn't work atm, because waf prints on stdout.

Then I believe the class should expose functions to pass through arbitrary text to gnuplot. These are all the AddExtra() functions I added.
The classes aim cannot be to have a representation of all styles, options, etc which gnuplot can do. Otherwise one could draw the plots directly using a plotting library, essentially skipping the textual representation step. Plainly speaking: just allow pass-through access to all/most of gnuplot functions.

The 2d/3d problem came up, because the "plot ..." was fixed in the GenerateOutput() function.

So a better solution would be splitting up the GnuplotDataset class into
Gnuplot2dDataset, Gnuplot2dFunction, Gnuplot3dDataset and, if needed, Gnuplot3dFunction.
All derived from abstract GnuplotDataset, which contains the common attributes (m_title, m_extra, ...)
I will do that this weekend.

> thanks for replying to my comments so quickly,

Thanks back, you're even faster :).

> Mathieu

Greetings,
Timo