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

Modified code to allow for stripping curves in Discount Curve that ha… #31

Merged
merged 2 commits into from Mar 19, 2016

Conversation

@tleitch
Copy link
Contributor

@tleitch tleitch commented Mar 18, 2016

…vefixed rate coupon frequency other than annual, fixed day count convetions other than Thirty360, and floating frequency reset other than 6 months. These are critical for adding basis curve impact for discounting.

…vefixed rate coupon frequency other than annual, fixed day count convetions other than Thirty360, and floating frequency reset other than 6 months. These are critical for adding basis curve impact for discounting.
@tleitch
Copy link
Contributor Author

@tleitch tleitch commented Mar 18, 2016

Dirk,
Here are changes in pull. Sorry I included Rcpp export as it blows the files up, but really there are 4 code files changes, discount.R (the calling function) discount.cpp (the RcPP called function) and curves.cpp as well as Rquanlib_internal.h . The three variables are a daycount paramter, a fixed coupon frequency parameter, and a floating reset frequency parameter. They are utilized in curves.cpp call to Quantlib.

I left the original rate helper and created a new definition for the curve stripper as the Bermudan option module expects the original version and I did not see a reason to mod the swaption code at this time.

Terry

@@ -59,6 +59,11 @@ DiscountCurve(params, tsQuotes, times)
that the largest time plus \code{dt} does not exceed the longest
maturity of the instruments used for calibration (no extrapolation).
}
\item{cpnParams}{A list specifying the \code{dayCounter} the day count convention for the
fixed leg (default is Thirty360), and \code{freq}, fixed coupon frequecny (defualt is Annua).

This comment has been minimized.

@eddelbuettel

eddelbuettel Mar 18, 2016
Owner

Annual, not Annua.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 18, 2016

That looks good at a first (uncaffeniated) glance. Sadly a lot of changes appear to just be whitespace which makes it a little noisy -- maybe we still have tabs in the files from way back when. There is also rquantlib-tl.Rproj which simply does not belong. I can clean that up, but if you could just 'git rm' it I'd appreciate it.

Also, leaving indentation of existing files rather than changing it is considered polite. I prefer 4 spaces just like QuantLib. See here and I also follow the R style (described in one of the manuals).

ChangeLog and NEWS entries would also be nice. I can add those.

But the bigger issue is: it is a clean extension which passes unit tests. Nice! I'll look more closely when I have a moment.

@tleitch
Copy link
Contributor Author

@tleitch tleitch commented Mar 18, 2016

Thanks! One thing you should be made aware of is the fact the unit test do not exercise (or exorcise) the fixed income routines. The "tests" directory has a master script Rquantlib.R and a matching output that I checked against, but while that script has a non-flat term structure, it isn't utilized. I checked old released version versus new version with the term structure in the file. To use the term structure in the file with the enclosed params you also need to set the evaluation date properly with setEvalutaionDate or the interpolation method will fail.

I'll set the spacing in my various RStudio's I'm using to develop (centos,osx,ubuntu and windows)

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 18, 2016

Look in inst/unitTests/ for the testing framework. It uses RUnit. I can walk you through.

Thanks for switching to 4 spaces. I think if you used the existing Rproj file it may work too...

@tleitch
Copy link
Contributor Author

@tleitch tleitch commented Mar 18, 2016

I removed the project file and fixed the typo and pushed, do I execute
another pull?

From: Dirk Eddelbuettel notifications@github.com
Reply-To: eddelbuettel/rquantlib
<reply+007042a2717f699a853e99e6b811c546286adc2321cbd96692cf000000011303bfe19
2a169ce087327ae@reply.github.com>
Date: Friday, March 18, 2016 at 9:08 AM
To: eddelbuettel/rquantlib rquantlib@noreply.github.com
Cc: terry leitch tleitch@uchicago.edu
Subject: Re: [rquantlib] Modified code to allow for stripping curves in
Discount Curve that ha� (#31)

Look in inst/unitTests/ for the testing framework. It uses RUnit. I can walk
you through.

Thanks for switching to 4 spaces. I think if you used the existing Rproj
file it may work too...


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#31 (comment)

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 18, 2016

No, GitHub is smarter than that. The page at #31 lists this discussion and also the fact that there are now two commits.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2016

Ok, I'll merge and post-process, restablished standard indentation and what not.

eddelbuettel added a commit that referenced this pull request Mar 19, 2016
Modified code to allow for stripping curves in Discount Curve that ha…
@eddelbuettel eddelbuettel merged commit ad2b783 into eddelbuettel:master Mar 19, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2016

Hm, why where the commits made by a Mark at a domain frutig.com which points to Wilmette, IL?

The file ChangeLog needs a name and email. I will put yours in for now. Should that be changed?

@tleitch
Copy link
Contributor Author

@tleitch tleitch commented Mar 19, 2016

not sure why his domain shows up, he worked on another project with me last year and we share some repositories on github. yes, use my name and email and I will look into and fix that going forward.

Sent from my iPhone

On Mar 19, 2016, at 8:34 AM, Dirk Eddelbuettel notifications@github.com wrote:

Hm, why where the commits made by a Mark at a domain frutig.com which points to Wilmette, IL?

The file ChangeLog needs a name and email. I will put yours in for now. Should that be changed?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@@ -166,15 +169,17 @@ QuantLib::DateGeneration::Rule getDateGenerationRule(const double n);
boost::shared_ptr<QuantLib::YieldTermStructure> buildTermStructure(Rcpp::List params, Rcpp::List);
QuantLib::Schedule getSchedule(Rcpp::List rparam);
boost::shared_ptr<QuantLib::FixedRateBond> getFixedRateBond(Rcpp::List bondparam, std::vector<double> ratesVec, Rcpp::List scheduleparam);
boost::shared_ptr<QuantLib::IborIndex> getIborIndex(Rcpp::List index, const QuantLib::Date today);
//boost::shared_ptr<QuantLib::IborIndex> getIborIndex(Rcpp::List index, const QuantLib::Date today);////******
/*boost::shared_ptr<QuantLib::IborIndex> getIborIndex(std:string type);////******/

This comment has been minimized.

@eddelbuettel

eddelbuettel Mar 19, 2016
Owner

What are you trying to say here with //****** ? To be revisited?

It is a real pain we don't have clean diff's. I am restoring whitespace to what it was so the diff between the next and the previous diff will be more meaningful.

@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2016

It will be an issue related to your github setup / account id etc. You may want to look into that going forward.

Almost done cleaning. Was a bit more painful than it needed to be, but now (at last ;-) I see your changes more clearly and they are nice and clean. We will get coding and indentation style etc sorted out.

eddelbuettel added a commit that referenced this pull request Mar 19, 2016
no code changes, small fixes to Rd file
eddelbuettel added a commit that referenced this pull request Mar 19, 2016
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2016

I pushed a new branch which I will probably merge into master shortly too. If you set my repository up as a remote 'parentrepo' (eg via something like

git remote add --track master parentrepo https://github.com/eddelbuettel/rquantlib.git

then

git pull --all
git merge parentrepo/master

will sync your fork. Right now you'd have to merge with the branch that has my changes.

eddelbuettel added a commit that referenced this pull request Mar 19, 2016
Feature/cleanup on #31
@eddelbuettel
Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 19, 2016

Master now has my #32 following this #31.

Onwards ... ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.