Modified bermudan option module to allow for swaptions other than 1 y…

[tleitch]

…ear option into 5 year swap. Modified documentation regarding new startDate and maturity inputs required for bermudan function call. Modified discountcurve by merging the floating and fixed paramters into a single list "legparams".

[tleitch]

The bermudanswaption() function has the start date and maturity hard coded as 1 year and 1+5 years. I modded the code to allow for general option expirations and swap maturities.

I also did not like the klunky way fix/float parameters are set in discount curve from my last pull request and merged them into a single structure and merged them into a single list "legparams"

[eddelbuettel]

Thanks for the submission! Do you see that this shows merge conflicts? Do you want to sort this out at your end -- presumably by syncing with the state of master in RQuantLib, resolving conflicts and the committing? Would make for a cleaner process...

[eddelbuettel]

Nice. I like that.

[eddelbuettel]

Not sure how those slipped in / survived for a decade. Ooops.

[tleitch]

I didn¹t bother to make it backward compatible��

[eddelbuettel]

I may consider that a show stopper.

[tleitch]

It's hard to make something hard coded backward compatible. I could go in and check if there's a null argument and replace it with 1/5 for startDate/maturity, but how many are using this in order to price a very specific expiration/maturity pair? For a function like this, I would argue default values might be a potential problem.

On the other hand, I modified the documented example to line up with the original 1/5 tenors tenor results that were the default results (though nowhere in the documentation are they listed as the assumed values).

[eddelbuettel]

Yes, it is a tradeoff. It wasn't immediately clear where the backwards compatibility breaking change occurred. Here we could argue it is a bug fix.

We can have it both ways. As I understand it, startDate and maturity should be but were not parameters. They were constants here. So we could (should ?) make them parameters, and consider defaulting them to 1 and 5. We can always do that with a warning a nice idiom is something like

f <- function(par) {
    if (missing(par)) {     # now we know
         par <- 42
         warning("You did not set par when calling f.  Setting default of 42.")
   }
   # get on with function ...
}

Not saying we must, just saying we can and that it would make sense to me. You are deeper in that code.

As for 'users must specify' versus 'sensible defaults' I generally like the latter....

[tleitch]

I can do a default with a warning. The only other issue with a default is it is specific to a calendar, so if the user is assuming a calendar, but I'll default it to US and flag that change as well.

[eddelbuettel]

Default and warning sounds good to me too.

[tleitch]

Pushed to my repository, do I perform a new pull?

[tleitch]

Sure. I forked your master and pulled to my machine, modded, and pushed, so not sure what I did wrong there. Will sort it.

On Mar 25, 2016, at 12:41 PM, Dirk Eddelbuettel notifications@github.com wrote:

Thanks for the submission! Do you see that this shows merge conflicts? Do you want to sort this out at your end -- presumably by syncing with the state of master in RQuantLib, resolving conflicts and the committing? Would make for a cleaner process...

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub #34 (comment)

[eddelbuettel]

Looks nice otherwise. I can merge by hand too -- which gets your commits in -- but that would not, I think, get the pull request in as such.

[eddelbuettel]

Do you want help getting it merged? I can do the manual steps outlined at the link here.

[tleitch]

If you could it would be a big help. I'm traveling with my family trying to have a holiday, it would be a big help.

[eddelbuettel]

Here is goes -- you seem to not have been current. I'll try to sort this out, at least in this branch.

edd@max:~/git/rquantlib(master)$ git checkout -b feature/merge-pr34
Switched to a new branch 'feature/merge-pr34'
edd@max:~/git/rquantlib(feature/merge-pr34)$ git pull https://github.com/tleitch/rquantlib.git master
remote: Counting objects: 17, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 17 (delta 15), reused 15 (delta 13), pack-reused 0
Unpacking objects: 100% (17/17), done.
From https://github.com/tleitch/rquantlib
 * branch            master     -> FETCH_HEAD
Auto-merging src/discount.cpp
CONFLICT (content): Merge conflict in src/discount.cpp
Auto-merging man/DiscountCurve.Rd
CONFLICT (content): Merge conflict in man/DiscountCurve.Rd
Auto-merging R/discount.R
CONFLICT (content): Merge conflict in R/discount.R
Automatic merge failed; fix conflicts and then commit the result.
edd@max:~/git/rquantlib(feature/merge-pr34)$ 

[eddelbuettel]

I am not really comfortable with this. Blocks seem to be missing, eg this from src/discount.cpp:

https://github.com/eddelbuettel/rquantlib/blob/master/src/discount.cpp#L96-L100

Including a closing brace. I don't like this.

Maybe a better approach would be to take the weekend off, and not rush this. I suggest that maybe you hold on to this PR to keep track of the changes you, but otherwise start with a clean checkout of master of my repo eg let's try to start from a clean sheet with commit 11583dc which is the most recent one.

[eddelbuettel]

Also once again all indentation is off. It cannot be so difficult to just load the RStudio project file to obtain four space indentation. This makes the comparison MUCH harder than it needs.

[eddelbuettel]

With the section I found as missing inserted, it builds and passes regression tests. R CMD check complains about the docs not being up to date with the updated interface:

* checking for code/documentation mismatches ... WARNING
Codoc mismatches from documentation object 'DiscountCurve':
DiscountCurve
  Code: function(params, tsQuotes, times = seq(0, 10, 0.1), legparams =
                 list(dayCounter = "Thirty360", freq = "Annual",
                 floatFreq = "Semiannual"))
  Docs: function(params, tsQuotes, times, fixParams, floatFreq)
  Argument names in code not in docs:
    legparams
  Argument names in docs not in code:
    fixParams floatFreq
  Mismatches in argument names:
    Position: 4 Code: legparams Docs: fixParams
  Mismatches in argument default values:
    Name: 'times' Code: seq(0, 10, 0.1) Docs: 

* checking Rd \usage sections ... WARNING
Undocumented arguments in documentation object 'DiscountCurve'
  ‘fixParams’ ‘floatFreq’
Documented arguments not in \usage in documentation object 'DiscountCurve':
  ‘legparams’

Functions with \usage entries need to have the appropriate \alias entries, and all their arguments documented.
The \usage entries must correspond to syntactically valid R code.
See chapter ‘Writing R documentation files’ in the ‘Writing R Extensions’ manual.
* checking Rd contents ... OK

So our procedures are not yet that well oiled.

[tleitch]

I agree, take the weekend off. Nit sure what happened as I forked the main and pulled into RStudio to mod. I was careful to make changes only to lines I was targeting and avoided any global re-indentation. Tabs were set to 4.

The changes were straightforward and I think I could do them again in an hour, so I propose I do a clean pull, mod it, and push again and see what we get.

[eddelbuettel]

If you look at RQuantLib now and notice the two branches, you can probably launch a 'Compare' similar to a pull request. That is what your next pull request should look: minimal and contained changes.

We will get there. It may take some practice.

I would also like you submit issue tickets before making breaking changes such as the leg timing. Some things are better discussed prior to making changes.

[eddelbuettel]

That became #35 for which which you can see the changes.

[tleitch]

Thank you.

[tleitch]

Ok, so I think I figured this out. I forked the main repository but pulled the code from the main repository to my linux box (headless, so no github app). I just did a pull request from the master to my master on my machine (which is what I should have done instead of fork), so now I will pull this down to my machine to code.

[eddelbuettel]

You seem confused. This has nothing to do with whether you use an app by github or not.

We use a repo here. If you "fork at GitHub" (say in the webclient) you create a remote copy of my repo in your GitHub account at github. At that point you desktop and laptop have identical status; you would just clone to either. I regularly work on at least three different computers for some repo (home, laptop, work) and it is totally seamless.

Please try a few of the existing git / github / ... tutorials. And/or play at Bitbucket or Gitlab (which allow you to have private repos so you can commit at will without anybody seeing it).