-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix calendar advance for unadjusted convention #1917
Fix calendar advance for unadjusted convention #1917
Conversation
Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it. |
c216c68
to
108de59
Compare
ql/time/calendar.cpp
Outdated
if (endOfMonth && isEndOfMonth(d)) | ||
return Calendar::endOfMonth(d1); | ||
|
||
if (endOfMonth && isEndOfMonth(d)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use Date::isEndOfMonth()
when the convention is Unadjusted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better, fixed
@@ -82,7 +82,7 @@ BOOST_AUTO_TEST_CASE(testConventions) { | |||
//Unadjusted | |||
SingleCase(SouthAfrica(), Unadjusted, Date(3,February,2015), Period(1,Months), false, Date(3,March,2015)), | |||
SingleCase(SouthAfrica(), Unadjusted, Date(3,February,2015), Period(4,Days), false, Date(9,February,2015)), | |||
SingleCase(SouthAfrica(), Unadjusted, Date(31,January,2015), Period(1,Months), true, Date(27,February,2015)), | |||
SingleCase(SouthAfrica(), Unadjusted, Date(31,January,2015), Period(1,Months), true, Date(28,February,2015)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igitur it's been a long time, but git tells me you wrote this test — do you think it makes sense to change the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lballabio Hi, yes, the fix looks correct. I probably used QuantlibXL to generate test data for the existing business day conventions, thereby including the bug in the test data. Sorry.
Congratulations on your first merged pull request! |
This PR resolves #1910
In Calendar::advance(), the return date will be moved to end of business day if endOfMonth && isEndOfMonth(d) is True. However, isEndOfMonth(d) is True when d is on or after the end of business day, which introduces a discrepancy between d and the return date.
This PR fixes this behavior by returning the end of calendar date when convention is Unadjusted.
I added a unit test to ensure this behaves correctly for a US treasury bond. The ref start date should be end of calendar date and the cashflow should match the real-world cashflow.