finmath / finmath-lib

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

General Overhaul of Forward Curves #21

Closed NiklasRodi closed 7 years ago

NiklasRodi commented 7 years ago
NiklasRodi commented 7 years ago

The existence of the method getForward(double fixingTime) is negotiable. I find it very handy for "standard" ForwardCurves as I only don't need a model here for 3/4 interpolation entities. However, I'm not familiar enough with ForwardCurveNelsonSiegelSvensson to oversee all effects here. So it might be safer that getForward(double fixingTime) throws a MethodNotImplementedException here

cfries commented 7 years ago

With respect to the getter getForward(double fixing) coming without a model I have a concern: If we allow this, than users might get used to it. That is they will be calling getForward without passing the model "in their code". But then, if you later want to "map" a forward curve to a discount curve (via a name) you have to rely on the fact that the user must call getForward(model, fixing) - otherwise it would be impossible to resolve (fetch) the discount curve. Note that we have a kind of "late" binding her. The discount curve is not specified as an object upon construction, it is specified by name! This is necessary to make the calibration work (and propagation of a change of the rate curve). Isn't it. So if you allow users to use getFoward() without model, you will not be able to replace that curve with a ForwardCurveFromDiscountCurve - isn't it?

I would even go further: Always require a non-null model.... (in that case the user must explicitly create a dummy model). What do you think?

Also: If you do not add this method, there is effectively one one file who has a change. So maybe we can get that one through and postpone the others...

I am happy with the other changes (except: sometime I don't see why the JavaDoc was changed or deleted.

cfries commented 7 years ago

Note: If you push changes to this branch, the pull request will be changed / adapted automatically - unless it is closed.

NiklasRodi commented 7 years ago

Wherever possible I have copied the parameter explanation from one constructor to the other (only deleting the parameters not used by the 2nd constructor) to eunsure that the "same" parameter gets the same Explanation Independent of the constructor. This has led to minor JavaDoc changes

cfries commented 7 years ago

I have removed the getForward(double) method.

cfries commented 7 years ago

LGTM

cfries commented 7 years ago

I will revert the change to the constructor public ForwardCurveFromDiscountCurve(String referenceDiscountCurveName, LocalDate referenceDate, String paymentOffsetCode) on how the name of the curve is constructed.

Reason:

There is an issue with this change. I realised it after merging, because Jenkins UnitTests were failing ( http://ci.finmath.net/job/finmath%20lib%20-%20java%206%20-%20github%20-%20maven%20build/200/ ). The issue here is the change in how the name of the forward curve is constructed. I would like to revert to the previous method where the name is constructed via

"ForwardCurveFromDiscountCurve(" + discountCurveName + "," + paymentOffsetCode + ")"

compared to the method introduced in this pull request:

referenceDiscountCurveName + "_asForwardCurve"

Example: Consider the case where you have one referenceDiscountCurve "discount-EUR" and you like to create two forward curves from it: one 3M and one 6M. The two forward curves will have the same name. Hence calling "model.addCurve" will store only one of them.

I don't see why the paymentOffsetCode was removed from the name.

In a future version it would be even better to make the name unique by adding a hash which also includes referenceDate, etc... but currently I don't know of any application where this is required.