Re: [Quantlib-dev] small improvements

[Luigi B. <lui...@gm...>]

On Mon, 2010-07-05 at 16:51 -0500, Kakhkhor Abdijalilov wrote:
> I was looking into QuantLib implementation recently and noticed
> several things which could be improved (IMHO). These aren't really
> bugs, more like small issues related to documentation, coding style
> and efficiency. Here is my list.

Kakhkhor,
	thanks for the report.  Here we go:


> =============================================================
> 1.    generalstatistics.hpp
>        In reset()  method use "the swap trick" to reset the vector of
> samples. The assignment
>         of an empty vector doesn't free the storage. (Item 17 in
> "Effective STL").

I'm not sure about that.  It's likely that after a reset(), we'll start
adding new samples, in which case the storage will save us an
allocation.


>       Doxygen for percentile() should say something like this:
>           Smallest x from the sample, such that P(X<=x) >= y.
> 
>       Doxygen for topPercentile() should say somthing like this:
>           Largest x from the sample, such that P(X>=x) >= y.

We should define P though. Do you care to give it a try?


> 2.    capfloortermvolcurve.hpp.
>        Use private inheritance from boost::noncopyable.

Done.


> 3.   longstaffschwartzpathpricer.hpp
>       It is very hard to understand what is going on here.
> 
>     Can we replace it with this simple loop?
> 
>     \code
>         for(Size i=0; i<n; ++i)
>             prices[i] = (*pathPricer_)(paths_[i], len-1);
>     \endcode

Done.

>     At the end of LongstaffSchwartzPathPricer::calibrate() use "the
> swap trick" to reset
>     the vector paths_, instead of using paths_.clear(). Clearing a
> vector doesn't free the storage.
>     The calibration of LongstaffSchwartzPathPricer requires tons of
> memory. We better to free
>     all the memory used by the vector paths_.

Done.


> 5.    instrument.hpp
>       "virtual" can be omitted from the declaration of 'performCalculations'.

No, that's intentional.  We wanted to leave it virtual so that one can
override the engine mechanism if needed.


> 6.  randomsequencegenerator.hpp
>      Document that both RNG::next() and RNG::nextInt32() are required.

Done.


> 7.  timergrid.cpp
>      replace times_.reserve(steps) with times_.reserve(steps+1), because times_
>      stores steps+1 values.

Done.

Thanks again,
	Luigi


-- 

These are my principles, and if you don't like them... Well, I have
others. 
-- Groucho Marx