Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding CDS bootstrapping #18

Merged
merged 56 commits into from
Dec 3, 2019
Merged

Conversation

farzadwp
Copy link
Contributor

In this commit I have

  • Added new classes to contain CDS, survival probabilities, and hazard rates curves;
  • Added new classes to contain CDS curve specifications;
  • Added generic methods to format and print for the above classes; and
  • Created bootsrapping generic methods

All of the new classes and methods contain full documentation with examples
@imanuelcostigan

Added:
New classes to contain CDS, survival probabilities, and hazard rates curves

New classes to contain CDS curve specifications

Added generic methods to format and print for the above classes

Created bootsrapping generic methods
Copy link
Owner

@imanuelcostigan imanuelcostigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions:

  1. Is there a nice way that you can combine the HazardCurve and SurvivalCurve classes into one? Looks like the only reason for separating them is because credule::bootstrapCDS() returns hazard rates and survival probabilities in one object and you want to distinguish them. Possibilities:
    • can you extend CDSCurve by including hazard_rates and survival_probabilities fields which are set to NA_numeric if the instance is not bootstrapped and otherwise the bootstrapped values? OR
    • define a class called BoostrappedCDSCurve which includes hazard_rates and survival_probabilities fields (plus others) which is returned by bootstrap() where the latter is a generic that replaces bootstrap_survprob() and bootstrap_harzardrate().
  2. Where do the valid rank labels come from? Would be better if we could make these nicer labels that have a consistent convention. All lower case would be better (the user doesn't have to remember). For eg. Senior -> "senior", SubTier3 -> "sub_tier_3", SubTier2Upper -> "sub_tier_2_upper", SubTier2Lower -> "sub_tier_2_lower", SubTier1 -> "sub_tier_1"

Actions:

  • I have added a few commits to yours on the imanuelcostigan:feature-cds-ic branch. (Long story about why I did this). Can you merge these into your fork? Happy to show you how to do this.
  • Please review my commit commentary to understand my changes.
  • Update your version of roxygen2
  • Use the print / format pattern of the ZeroCurve class for the various credit curve classes you have introduced (main thing is that it goes via a tibble).
  • Document whether the probabilities passed to SurvivalProbCurve are cumulative or marginal survival probabilities
  • Review the CONTRIBUTING file on the repo as it relates to PR and make sure you have added test cases, updated the NEWS file, etc
  • Update the README.Rmd (and knit the corresponding README.md file)
  • Create a vignette for the credit curve functionality
  • Ensure there are no errors, warnings or notes returning from devtools::check()

@imanuelcostigan imanuelcostigan changed the base branch from master to feature-cds July 7, 2019 02:56
@imanuelcostigan imanuelcostigan changed the base branch from feature-cds to master July 7, 2019 04:14
farzadwp and others added 23 commits July 7, 2019 14:19
Added:
New classes to contain CDS, survival probabilities, and hazard rates curves

New classes to contain CDS curve specifications

Added generic methods to format and print for the above classes

Created bootsrapping generic methods
* Removed duplicate SubTier1 value
* Ordered ranks
* Use inheritParams to avoid repeating text
* Export constructor
* Use lowercase argument names (LGD -> lgd)
* Assert that tenor and spread arguments to have same lengths (no need to recycle vector values to ensure equivalent lengths).
* Improve documentation of CDSCurve
* Improve style of code and documentation
* using methods from `assertthat`
* ensure that tenors and probabilities are the same length
Prefer not to have concatenated words in method / class names
@farzadwp
Copy link
Contributor Author

1- In this iteration, the new credit classes mirror interest rate classes in functionality, as follows:
SurvivalProbabilities <-> DiscountFactors
ZeroHazardRate <-> InterestRate
CreditCurve <-> ZeroCurve

2- I have limited the use of Credule package (for bootstrapping) to only one generic: as_SurvivalProbabilities.CDSCurve

3- CreditCurve constructor only needs Survival Probabilities, so it doesn't have any dependency on the bootstrapping. I chose the name CreditCurve instead of the suggested BootstarppedCDSCurve, because the class no longer depends on bootstrapping.

4- Zero Hazard rates are now calculated from Survival Probabilities, without having to do an additional bootstrapping. (This is to ensure internal consistency in terms of business conventions)

5- I have extended all of the interpolation generic functions, to mimic similar functionalities for the credit classes. Here, note that
I overload interpolate_dfs with CreditCurve, instead of defining a new generic (e.g. interpolate_sps)

6- There are additional rules for CDSSpecs in cred-ops. When output specs are ambiguous, they are replaced with an "Empty" CDSSpecs object.

7- for +,-,/ and *, I included an assertion to check if the length of objects are equal. In IR and DF operators, if the length are not equal, equally lengthed vectors are created. I suggest including an assertion for IR and DF operators as well.


Point-to-point responses to comments:

Question 1- In this iteration, Credit Curve holds that responsibility, in the same way that ZeroCurve does for rates.
Question 2- Discussed in the meeting
Action 1 - Done
Action 2- Done
Action 3- I updated my Roxygen2 to 6.1.0
Action 4- Done
Action 5- I added this to as_SurvivalProbabilities.CDSCurve
Action 6- I added some tests on the important functions. Note that context() is being deprecated, so I did not use it.
Action 7- Done
Action 8- Check() fails on running fmbasics-Ex.R, which I did not edit. I haven't been able to figure out the cause of the issue.

@imanuelcostigan imanuelcostigan self-requested a review September 7, 2019 04:21
Copy link
Owner

@imanuelcostigan imanuelcostigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please:

  1. refresh your PR from master and resolve conflicts
  2. make sure all checks pass with no warnings, notes etc

@farzadwp
Copy link
Contributor Author

Please review the pull request. I have
1- refreshed your PR from master and resolve conflicts
2- resolved errors, warnings, and notes

@imanuelcostigan imanuelcostigan merged commit f9a79f0 into imanuelcostigan:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants