small improvements
Locked
 
|
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. ============================================================= 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"). 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. 2. capfloortermvolcurve.hpp. Use private inheritance from boost::noncopyable. 3. longstaffschwartzpathpricer.hpp It is very hard to understand what is going on here. \code std::transform(paths_.begin(), paths_.end(), prices.begin(), boost::bind(&EarlyExercisePathPricer<PathType> ::operator(), pathPricer_.get(), _1, len-1)); \endcode Can we replace it with this simple loop? \code for(Size i=0; i<n; ++i) prices[i] = (*pathPricer_)(paths_[i], len-1); \endcode 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_. 5. instrument.hpp "virtual" can be omitted from the declaration of 'performCalculations'. 6. randomsequencegenerator.hpp Document that both RNG::next() and RNG::nextInt32() are required. 7. timergrid.cpp replace times_.reserve(steps) with times_.reserve(steps+1), because times_ stores steps+1 values. ============================================================= Regards, Kakhkhor Abdijalilov. ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ QuantLib-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/quantlib-dev |
|
|
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 ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ QuantLib-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/quantlib-dev |
|
|
>> 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. Agree. The storage can be reused later. In that case, we could simply use vector's clear method instead of assigning an empty vector. >We should define P though. Do you care to give it a try? I would, but never used doxygen before. >> 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. It is not a bug, but more like pedantic adherence to the coding style. performCalculations is declared virtual in LazyObject. We could drop the keyword virtual when overwriting performCalculations in Instrument, just like we drop the keyword virtual when overwriting all other virtual methods in derived classes. Regards, Kakhkhor Abdijalilov. ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ QuantLib-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/quantlib-dev |
|
|
On Mon, 2010-07-12 at 15:47 -0500, Kakhkhor Abdijalilov wrote:
> >We should define P though. Do you care to give it a try? > I would, but never used doxygen before. No problem, just send me the text. > >> 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. > > It is not a bug, but more like pedantic adherence to the coding style. > performCalculations is declared virtual in LazyObject. Sorry, I though you meant to declare it as non-virtual. You're right, it's redundant. It doesn't hurt though. Luigi -- When I was a boy of fourteen, my father was so ignorant I could hardly stand to have the old man around. But when I got to be twenty-one, I was astonished at how much the old man had learned in seven years. -- Mark Twain ------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first _______________________________________________ QuantLib-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/quantlib-dev |
«
Return to quantlib-dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |