Latest commits

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Latest commits

Luigi Ballabio-2
Andre,
        first things first: it's a great job you did on date calculations and all
the other stuff, so kudos to you.

Secondly, I'm going to nag you just a tiny bit---feel free to come after me
with a big clown-sized rubber hammer...

In random order:

a) Calendar::isLastBusinessDayOfMonth seems quite a lot of writing
(although I'm sure I came up with names just as long in the past, so I
ain't throwing the first stone here). How about Calendar::isEndOfMonth?

b) As a matter of personal taste entirely, and therefore fully discardable
on your part, I'm not enthusiastic about Date::Date(string). I'd rather
keep the class clear from IO (Ditto for Period.) But I can live with it.

(Aside: if the constructor was prompted by:

string s1, s2;
cin >> s1 >> s2;
Date d1(s1,fmt);
Date d2(s2,fmt);

how about we implement operator>> and an IO manipulator instead, so that
one can write:

cin >> setdateformat(fmt);
Date d1, d2;
cin >> d1 >> d2;

In the other cases, writing "Date d = DateParser::parse(s,fmt)" hurts me
less, but that's only me :) end of aside)

c) Maybe Period::operator== should be a partial function. There's a subset
of the domain where it is well defined ("1 year == 12 months" is ok, and so
is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days == 1
month"? Or "4 weeks == 1 month"? With the current implementation, the first
is false, with the disconcerting result that if d = Date(22,7,2003), p1 !=
p2 but d.plus(p1) == d.plus(p2). The second is true, but for most dates,
d.plus(p1) != d.plus(p2). How about throwing an exception if the == is
undecidable?

d) Date::ascending() is none other than std::less<Date>(). In the same way,
Date::descending() is std::greater<Date>() and Date::less_equal(d) is
std::bind2nd(std::less_equal<Date>(),d). If those classes are to be used
with the STL, probably we better use their STL implementation :) Especially
Date::less_equal might be more confusing than useful, since std::less_equal
is a binary function. And in general, when one familiar with the STL reads
std::less<Date>, he knows what that it. If he reads Date::ascending, he
doesn't know. The same goes for TimeBasket::Entry.

e) Scheduler. No problem per se---on the contrary, I'm grateful you tackled
the thing---but it starts to have too many optional parameters. We could
make it a bit prettier with the named parameter idiom, i.e.,

Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d);
Scheduler s2 = MakeScheduler(mandatory parameters).backwards();
Scheduler s3 =
     MakeScheduler(mandatory parameters).withStubDate(d).backwards();

f) CashFlowVectors---a lot of parameters now. Maybe we should pass them a
Scheduler? The same goes for SimpleSwap.

g) Swap::fairRate seems to work only if the first leg is the fixed one---I
wouldn't count on that. Also, Swap is a generic swap class: one could be
swapping two floating legs, so that fairRate() might not make sense at all.

Thanks again,
                Luigi



Reply | Threaded
Open this post in threaded view
|

Re: Latest commits

Ferdinando M. Ametrano-2

Andre's latest commit is the right occasion for me to urge everybody to
consider adding a unit test when adding new features/classes. It is easier
done than said, and it will guarantee the author that next releases will be
compliant to the original test-specification.

ciao -- Nando



Reply | Threaded
Open this post in threaded view
|

Re: Latest commits

Luigi Ballabio-2
In reply to this post by Luigi Ballabio-2
At 4:27 PM +0200 7/22/03, Luigi Ballabio wrote:
>In random order:

I forgot h) please oh please, 4-spaces tabs... :)

Did we find out some way to make this comfortable for you? Maybe some
emacs macro that makes the setting local for the buffer?

Later,
        Luigi


Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Andre Louw-2
In reply to this post by Luigi Ballabio-2
Luigi,

> a) Calendar::isLastBusinessDayOfMonth seems quite a lot of writing. How
about Calendar::isEndOfMonth?

No problem, the original reason for the long name is to clarify that it's
not just last day of month, but last _business_ day of month, but then
that's really superfluous where a calendar is concerned!

> b) As a matter of personal taste entirely, and therefore fully discardable
on your part,
> I'm not enthusiastic about Date::Date(string). I'd rather keep the class
clear from IO (Ditto for Period.)

I did this solely to make my life in SWIG easier, but I'll put my back into
it and clean up.

> But I can live with it.

There are many things we can live with, that does not necessarily make them
right!

> how about we implement operator>> and an IO manipulator instead, so that
one can write:
> cin >> setdateformat(fmt);
> Date d1, d2;
> cin >> d1 >> d2;

Will look into it.

> c) Maybe Period::operator== should be a partial function.

Not quite sure I understand what you mean?

> There's a subset of the domain where it is well defined ("1 year == 12
months" is ok, and so
> is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days ==
1 month"?
> Or "4 weeks == 1 month"? How about throwing an exception if the == is
undecidable?

Will do.

> d) Date::ascending() is none other than std::less<Date>(). In the same
way,
> Date::descending() is std::greater<Date>() and Date::less_equal(d) is
> std::bind2nd(std::less_equal<Date>(),d).

Shows my lack of STL knowledge - alas, peer review is a way of learning!

> e) Scheduler. No problem per se---on the contrary, I'm grateful you
tackled
> the thing---but it starts to have too many optional parameters. We could
> make it a bit prettier with the named parameter idiom ...
> f) CashFlowVectors---a lot of parameters now. Maybe we should pass them a
> Scheduler? The same goes for SimpleSwap.

OK, once again I learn...

> g) Swap::fairRate seems to work only if the first leg is the fixed one---I

> wouldn't count on that. Also, Swap is a generic swap class: one could be
> swapping two floating legs, so that fairRate() might not make sense at
all.

Sorry, I realized this and should have removed it before I checked in.

Thanks for your input Luigi


Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Andre Louw-2
In reply to this post by Luigi Ballabio-2
Nando,

> Andre's latest commit is the right occasion for me to urge
> everybody to consider adding a unit test when adding new features/classes.

> It is easier done than said, and it will guarantee the author that next
> releases will be compliant to the original test-specification.

Will do.

Andre


Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Andre Louw-2
In reply to this post by Luigi Ballabio-2
Luigi,

Some questions on the so-called "named parameter paradigm" as per your
comments on Scheduler.

Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d);
Scheduler s2 = MakeScheduler(mandatory parameters).backwards();
Scheduler s3 = MakeScheduler(mandatory
parameters).withStubDate(d).backwards();

Am I correct to assume "withStubDate" would be defined something like

> Scheduler withStubDate(const Date& date) {
> stubDate_ = date;
> return *this;
> }

On the other hand this could erroneously be assumed to be a constructor,
which of course it isn't! Am I missing something? Also, some flag would need
to be set to force creation/recreation of the date vector?

Thanx
Andre


Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Jens Thiel-2
In reply to this post by Luigi Ballabio-2
Hi there,

one idea here: implement < and > for the cases were it is well defined, and
expand == to be "not larger and not smaller", eg.

  4w<1m false
  4w>1m false

  4w==1m expands to ( !(4w<1m) && !(4w>1m) ) which is true

  28d<1m false
  31d>1m false
 
  30d==1m expands to ( !(30d<1m) && !(30d>1m) ) which is true

This could also be done in a Compare() function that returns either -1, 0 or
1 to make sure that absolutely noone uses the operators carelessly.

Regards,

Jens.


c) Maybe Period::operator== should be a partial function. There's a subset
of the domain where it is well defined ("1 year == 12 months" is ok, and so
is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days == 1
month"? Or "4 weeks == 1 month"? With the current implementation, the first
is false, with the disconcerting result that if d = Date(22,7,2003), p1 !=
p2 but d.plus(p1) == d.plus(p2). The second is true, but for most dates,
d.plus(p1) != d.plus(p2). How about throwing an exception if the == is
undecidable?



Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Luigi Ballabio-2
In reply to this post by Andre Louw-2
At 12:51 PM 7/23/03 +0200, Andre Louw wrote:
>Some questions on the so-called "named parameter paradigm" as per your
>comments on Scheduler.
>
>Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d);
>Scheduler s2 = MakeScheduler(mandatory parameters).backwards();
>Scheduler s3 = MakeScheduler(mandatory
>parameters).withStubDate(d).backwards();

It would be something like:

class MakeScheduler {
   private:
     ... (mandatory parameters)
     Date stub;
     bool fromEnd;
     ... (other optional parameters)
   public:
     MakeScheduler(mandatory params) : copy mandatory parameters,
         fromEnd(false), ... {}
     MakeScheduler& withStubDate(const Date& d) { stub = d; return *this; }
     MakeScheduler& backwards() { fromEnd = true; return *this; }
     MakeScheduler& forwards() { fromEnd = false; return *this; }
     // when we're ready...
     operator Scheduler() {
         return Scheduler(mandatory params, stub, fromEnd, ...);
     }
};

i.e., no Scheduler is actually instantiated until the MakeScheduler thing
is assigned to one.

Bye,
         Luigi




Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Luigi Ballabio-2
In reply to this post by Jens Thiel-2
At 12:37 PM 7/23/03 +0200, Jens Thiel wrote:

>one idea here: implement < and > for the cases were it is well defined, and
>expand == to be "not larger and not smaller", eg.
>
>   4w<1m false
>   4w>1m false
>
>   4w==1m expands to ( !(4w<1m) && !(4w>1m) ) which is true
>
>   28d<1m        false
>   31d>1m        false
>
>   30d==1m expands to ( !(30d<1m) && !(30d>1m) ) which is true

Hmm, I still have the problem that:

Date d(23,7,2003);
Period p1(30,Days);
Period p2(4,Weeks);
Period p3(1,Months);

p1 == p3                 => true
d.plus(p1) == d.plus(p3) => false
p2 == p3                 => true
d.plus(p2) == d.plus(p3) => false

which is kind of surprising, and the additional problem that equality is
not transitive:

30d == 1m  => true
1m == 4w   => true
30d == 4w  => false

Later,
         Luigi



Reply | Threaded
Open this post in threaded view
|

Re: RE: Latest commits

Luigi Ballabio-2
In reply to this post by Andre Louw-2
Hi Andre,

At 09:24 AM 7/23/03 +0200, Andre Louw wrote:
> > b) As a matter of personal taste entirely, and therefore fully discardable
>on your part,
> > I'm not enthusiastic about Date::Date(string). I'd rather keep the class
>clear from IO (Ditto for Period.)
>
>I did this solely to make my life in SWIG easier, but I'll put my back into
>it and clean up.

We can leave the additional constructor in SWIG---it just takes to write

%extend Date {
     Date(const std::string& s, const std::string& fmt) {
         return new Date(DateParser::parse(s,fmt));
     }
}


> > c) Maybe Period::operator== should be a partial function.
>
>Not quite sure I understand what you mean?

I slipped into math talk--or maybe some other field? I meant "not defined
upon the whole domain", since there are pairs of Periods for which it is
ambiguous.

Bye,
         Luigi



Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Andre Louw-2
In reply to this post by Luigi Ballabio-2
Luigi,

> CashFlowVectors---a lot of parameters now. Maybe we should pass them a
Scheduler?
> The same goes for SimpleSwap.

Would you prefer me to keep the original constructors (before adding the
optional parameters)?

Andre


Reply | Threaded
Open this post in threaded view
|

Re: RE: Latest commits

Luigi Ballabio-2
At 03:25 PM 7/23/03 +0200, Andre Louw wrote:
> > CashFlowVectors---a lot of parameters now. Maybe we should pass them a
>Scheduler?
> > The same goes for SimpleSwap.
>
>Would you prefer me to keep the original constructors (before adding the
>optional parameters)?

Yes, I'd keep them for the time being. We can deprecate them in this
release and remove them in the next one.

Bye,
         Luigi




Reply | Threaded
Open this post in threaded view
|

RE: Latest commits

Andre Louw-2
In reply to this post by Luigi Ballabio-2
Luigi,

> class MakeScheduler {
> ...
>     operator Scheduler() {
>         return Scheduler(mandatory params, stub, fromEnd, ...);
>     }
> };

Is there a nice way of implementing the conversion operator in SWIG? Any
examples in QuantLib?

Thanx
Andre


Reply | Threaded
Open this post in threaded view
|

Re: RE: Latest commits

Luigi Ballabio-2
At 11:35 AM 7/25/03 +0200, Andre Louw wrote:
> > class MakeScheduler {
> >       ...
> >     operator Scheduler() {
> >         return Scheduler(mandatory params, stub, fromEnd, ...);
> >     }
> > };
>
>Is there a nice way of implementing the conversion operator in SWIG? Any
>examples in QuantLib?

As for Python, there's no need to export MakeScheduler since it has keyword
arguments---one can write

s = Scheduler(..., fromEnd = 1)
s = Scheduler(..., stub = Date(...))

For the other languages, I'll have to think about it...

Later,
         Luigi