Conversation
cfries
left a comment
cfries
left a comment
There was a problem hiding this comment.
The PR / MR contains some good aspects (e.g. introducing more modularity in extrapolation) but there are some changes which I found unclear: Better submit smaller PRs that are specific to a single extentions. As I would not like to introduce the change in the GammaProcess I would close this one.
|
|
||
|
|
||
| private static final long serialVersionUID = 8413020544732461630L; | ||
| private transient RandomVariable[][] gammaIncrements; |
There was a problem hiding this comment.
I believe it is not good to move the gammaIncrements to the RandomVariableLazyEvaluation. The RandomVariableLazyEvaluation is not linked to the gamma distribution. LazyEval is a property of the implementation, leaving the distributional property open. Init should be driven by an operator (supplier).
| * For performance reasons we return directly the stored data (no defensive copy). | ||
| * We return an immutable object to ensure that the receiver does not alter the data. | ||
| */ | ||
| return gammaIncrements[timeIndex][factor]; |
There was a problem hiding this comment.
I appears as if you initalize the gammaIncrements above (see Line 122), but then repeat the work by initallizing the RandomVariableLazyEvaluation.
| discountCurve = DiscountCurveInterpolation.createDiscountCurveFromDiscountFactors(discountCurveName, new double[] { 0.0 }, new double[] { 1.0 }); | ||
| model = model.addCurves(discountCurve); | ||
| } | ||
| DiscountedAnalyticCurves discountedAnalyticCurves = new DiscountedAnalyticCurves(); |
There was a problem hiding this comment.
The createDiscountCurve is a private method, because it is used during the calibration. At that point the return of the a curve with just one point (0.0 -> 1.0) is a desired feature.
Moving this to a public class DiscountedAnalyticCurves may expose unexpected behaviour. Not 100 % sure why we need a separate class. Also the name is maybe suboptimal "discounted analytic curves" (why -ed and why plural?).
2 participants
Set #1
Location - src\main\java\net\finmath\fouriermethod\calibration\models\CalibratableMertonModel.java
method name - getCharacteristicFunctionModel()
Prev commit - 93620a2
This commit - 399f245
Maven compile status - success
Line : 104
Explanation : In this refactored code, each field of the descriptor object is assigned to an explaining variable with a descriptive name. The method then returns a new MertonModel object constructed using these variables. The refactoring has improved the readability and
understandability of the code.
Location - src\main\java\net\finmath\marketdata\products\MarketForwardRateAgreement.java
method - getValue()
Prev commit - 399f245
This commit - a0ff0fb
Maven compile status - success
Line : 62
Explaination :
While running Designite - it picked up a complex conditional smell -
“if(forwardCurve == null && forwardCurveName != null && forwardCurveName.length() > 0)”
This was refactored by adding another layer of abstraction and encapsulating the if conditional logic, to another method -
“ public boolean checkForwardCurve(ForwardCurve forwardCurve, String forwardCurveName){
return (forwardCurve == null && forwardCurveName != null && forwardCurveName.length() > 0);
}
”
Now the if becomes - if(checkForwardCurve(forwardCurve, forwardCurveName))
Location : src\main\java\net\finmath\timeseries\models\parametric\DisplacedLognormalARMAGARCH.java
method : getLogLikelihoodForParameters()
Prev commit : a0ff0fb
This commit : 9535176
Maven compile status : success
Line : 92
Explanation : In this version, variables like evalPrev, eval, value1, and value2 have been renamed to evaluationPrev, evaluation, currentValue, and nextValue, respectively, to make their purpose more clear.
Set #2
Location : src\main\java\net\finmath\marketdata\calibration\CalibratedCurves.java
method : createDiscountCurve()
Prev commit : 9535176
This commit : 0ab8b66
Maven compile status : success
Line : 758
Explanation : the class CalibratedCurves had a feature envy problem due to the fact that this class had multiple responsibilities assigned to it, Here the class had the responsibility of creating calibrated curves and discounted analytic curves.By following the "Extract Class" refactoring. I have moved the method createDiscountCurve to a new class DiscountedAnalyticCurves and made it a public method.
By doing this, I have improved the code's readability and maintainability. The code now has a clear separation of concerns, and the CalibratedCurves class no longer has a feature envy issue. The DiscountedAnalyticCurves class now has a single responsibility of creating discount curves from an analytic model.
Additionally, I have reduced the coupling between the CalibratedCurves and AnalyticModel classes by passing the AnalyticModel as a parameter to the createDiscountCurve method instead of accessing it through a field. This makes the DiscountedAnalyticCurves class more reusable and testable.
New added class Location : src\main\java\net\finmath\marketdata\calibration\DiscountedAnalyticCurves.java
Location : src\main\java\net\finmath\interpolation\RationalFunctionInterpolation.java
method - getValue()
Prev commit - 0ab8b66
This commit - ea7248c
Maven compile status - success
Explanation -
To replace the conditional with polymorphism, I identified the common behaviour between the different types of extrapolation methods and then create a superclass that contains this behaviour. Then, I create a subclass for each type of extrapolation method and move the behaviour specific to each method to its respective subclass.
Created an abstract superclass Extrapolation: location - src\main\java\net\finmath\interpolation\Extrapolation.java
Created a subclass for each type of extrapolation method and implement the specific behaviour in each subclass:
Location - src\main\java\net\finmath\interpolation\LinearExtrapolation.java
Location - src\main\java\net\finmath\interpolation\ConstantExtrapolation.java
With this refactored code, we have replaced the conditional logic for extrapolation with a polymorphic approach, making the code easier to understand and maintain.
Location : src\main\java\net\finmath\montecarlo\GammaProcess.java
method : getIncrement()
line: 118
Moved method to : src\main\java\net\finmath\montecarlo\RandomVariableLazyEvaluation.java
line: 111
Prev commit - ea7248c
This commit - 64f8845
Maven compile status - success
Explanation - The Gamma method had getIncreament method in it which was returning a random variable, It also follows lazy initialization and there was already a class for randomVariablesLazyEvaluation.
I moved the method to that class to ensure better code maintainability and readablity.
ALL COMMITS :
commit 64f8845 (HEAD -> code-refactoring)
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 21:36:23 2023 -0300
commit ea7248c
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 18:07:56 2023 -0300
commit 0ab8b66
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 16:31:25 2023 -0300
commit 9535176
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 14:59:04 2023 -0300
commit a0ff0fb
Author: Alen John al283652@dal.ca
Date: Wed Apr 5 14:35:44 2023 -0300