finmath / finmath-lib

Mathematical Finance Library: Algorithms and methodologies related to mathematical finance.
Apache License 2.0
488 stars 168 forks source link

Overhaul of Schedule #22

Closed NiklasRodi closed 7 years ago

NiklasRodi commented 7 years ago
NiklasRodi commented 7 years ago

I explicitly did not replace e.g. modelDcc = new DayCountConvention_ACT_365() in CurveFactory as I did not want to change any modelling dcc but only the internal (representation) dcc

NiklasRodi commented 7 years ago

For a DiscountCurve the toString() would look something like this: discountcurve_tostring

NiklasRodi commented 7 years ago

For a ForwardCurve the toString() would look something like this: forwardcurve_tostring

NiklasRodi commented 7 years ago

Note that a ForrwardCurveFromDiscountCurve shows no points as it does not have own points (only actually belong to referenceDiscountCurveForForwards)

cfries commented 7 years ago

I assume that the implementation of getDateFromDouble has a bug. For some days you will lose a day due to rounding (and then create an exception). Just try the following:

double x = 3.0
double f = x / 365.0;
int i = (int)(f*365);
cfries commented 7 years ago

Hi.

I have committed three changes to the PR (please pull/update):

The reason to use a separate class: From the discussion I had (also with others) there schould be a central place where the conversion from Date to Double is documented (JavaDoc). Example: People using an Equity Model would likely not check the documentation/methods of schedule. Since this conversion is fundamental, it should be documented and be performed in a separate place.

I have just added the class, but not yet changed the code of your PR.

finmath commented 7 years ago

Being public final versus ´public´ but not final: "violation of immutability" the user could change the field, hence create a schedule with ACT/360, then change it back to ACT/365 (at maybe another place) and get strange results. Also consider two threads running in parallel both using the field.

Being public versus private: "violation of encapsulation“ being private we ensure that the user has to call the conversion method and it is possible to perform checks. Only using the two conversion methods we can always intercept (e.g. check for rounding or allow future improvements or changes). Why making it public without need. If there is no argument for public one shouldn’t go for it.

Am 17.05.2017 um 11:37 schrieb NiklasRodi notifications@github.com:

@NiklasRodi commented on this pull request.

In src/main/java6/net/finmath/time/Schedule.java https://github.com/finmath/finmath-lib/pull/22#discussion_r116954037:

@@ -29,7 +29,7 @@ */ public class Schedule implements ScheduleInterface {

  • private static DayCountConventionInterface internalDayCounting = new DayCountConvention_ACT_365();
  • public static DayCountConventionInterface internalDayCounting = new DayCountConvention_ACT_365(); // static internal daycount convention to convert between doubles and date diffs Yes this should be private. Why not make it final?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/finmath/finmath-lib/pull/22#discussion_r116954037, or mute the thread https://github.com/notifications/unsubscribe-auth/ADpJQRRR3w0oKl3sHs1Z0waQ4NbOJ25iks5r6r-9gaJpZM4NbHK1.

cfries commented 7 years ago

The pretty printing in the toString is nice (e.g. for viewing curve in the object browser). I have fixed the NPE and made a small change to the print output. Please check if this suits you.

UnitTest should pass now...