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

Added discrete dividend interface to EuropeanOption and AmericanOption #73

Merged
merged 3 commits into from Oct 26, 2016

Conversation

Projects
None yet
2 participants
@hfty
Contributor

hfty commented Oct 24, 2016

No description provided.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

That looks like a very fine piece of work on first try -- a little hard to see all from the diff but no smoking gun yet.

Owner

eddelbuettel commented Oct 24, 2016

That looks like a very fine piece of work on first try -- a little hard to see all from the diff but no smoking gun yet.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

Ahh, but I spoke too soon. We do have a smoking gun in that tests failed:

vanilla.cpp: In function ‘Rcpp::List europeanOptionEngine(std::string, double, double, double, double, double, double, std::vector<double>, std::vector<double>)’:
vanilla.cpp:68:90: error: no matching function for call to ‘std::vector<QuantLib::Date>::push_back(boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type)’
             discreteDividendDates.push_back(today.dateTime() + discreteDividendLengths[i]);
                                                                                          ^
vanilla.cpp:68:90: note: candidate is:
In file included from /usr/include/c++/4.8/vector:64:0,
                 from /usr/include/boost/detail/container_fwd.hpp:93,
                 from /usr/include/boost/functional/hash/extensions.hpp:17,
                 from /usr/include/boost/functional/hash/hash.hpp:540,
                 from /usr/include/boost/functional/hash.hpp:6,
                 from /usr/include/boost/unordered/unordered_set.hpp:20,
                 from /usr/include/boost/unordered_set.hpp:16,
                 from /usr/include/ql/patterns/observable.hpp:37,
                 from /usr/include/ql/event.hpp:29,
                 from /usr/include/ql/cashflow.hpp:28,
                 from /usr/include/ql/quantlib.hpp:10,
                 from ../inst/include/rquantlib_internal.h:24,
                 from ./rquantlib.h:33,
                 from vanilla.cpp:22:
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note: void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = QuantLib::Date; _Alloc = std::allocator<QuantLib::Date>; std::vector<_Tp, _Alloc>::value_type = QuantLib::Date]
       push_back(const value_type& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note:   no known conversion for argument 1 from ‘boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type {aka boost::posix_time::ptime}’ to ‘const value_type& {aka const QuantLib::Date&}’
vanilla.cpp: In function ‘Rcpp::List americanOptionEngine(std::string, double, double, double, double, double, double, int, int, std::string, std::vector<double>, std::vector<double>)’:
vanilla.cpp:175:90: error: no matching function for call to ‘std::vector<QuantLib::Date>::push_back(boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type)’
             discreteDividendDates.push_back(today.dateTime() + discreteDividendLengths[i]);
                                                                                          ^
vanilla.cpp:175:90: note: candidate is:
In file included from /usr/include/c++/4.8/vector:64:0,
                 from /usr/include/boost/detail/container_fwd.hpp:93,
                 from /usr/include/boost/functional/hash/extensions.hpp:17,
                 from /usr/include/boost/functional/hash/hash.hpp:540,
                 from /usr/include/boost/functional/hash.hpp:6,
                 from /usr/include/boost/unordered/unordered_set.hpp:20,
                 from /usr/include/boost/unordered_set.hpp:16,
                 from /usr/include/ql/patterns/observable.hpp:37,
                 from /usr/include/ql/event.hpp:29,
                 from /usr/include/ql/cashflow.hpp:28,
                 from /usr/include/ql/quantlib.hpp:10,
                 from ../inst/include/rquantlib_internal.h:24,
                 from ./rquantlib.h:33,
                 from vanilla.cpp:22:
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note: void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = QuantLib::Date; _Alloc = std::allocator<QuantLib::Date>; std::vector<_Tp, _Alloc>::value_type = QuantLib::Date]
       push_back(const value_type& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note:   no known conversion for argument 1 from ‘boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type {aka boost::posix_time::ptime}’ to ‘const value_type& {aka const QuantLib::Date&}’
make: *** [vanilla.o] Error 1
ERROR: compilation failed for package ‘RQuantLib’
* removing ‘/home/travis/build/eddelbuettel/rquantlib/RQuantLib.Rcheck/RQuantLib’
Owner

eddelbuettel commented Oct 24, 2016

Ahh, but I spoke too soon. We do have a smoking gun in that tests failed:

vanilla.cpp: In function ‘Rcpp::List europeanOptionEngine(std::string, double, double, double, double, double, double, std::vector<double>, std::vector<double>)’:
vanilla.cpp:68:90: error: no matching function for call to ‘std::vector<QuantLib::Date>::push_back(boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type)’
             discreteDividendDates.push_back(today.dateTime() + discreteDividendLengths[i]);
                                                                                          ^
vanilla.cpp:68:90: note: candidate is:
In file included from /usr/include/c++/4.8/vector:64:0,
                 from /usr/include/boost/detail/container_fwd.hpp:93,
                 from /usr/include/boost/functional/hash/extensions.hpp:17,
                 from /usr/include/boost/functional/hash/hash.hpp:540,
                 from /usr/include/boost/functional/hash.hpp:6,
                 from /usr/include/boost/unordered/unordered_set.hpp:20,
                 from /usr/include/boost/unordered_set.hpp:16,
                 from /usr/include/ql/patterns/observable.hpp:37,
                 from /usr/include/ql/event.hpp:29,
                 from /usr/include/ql/cashflow.hpp:28,
                 from /usr/include/ql/quantlib.hpp:10,
                 from ../inst/include/rquantlib_internal.h:24,
                 from ./rquantlib.h:33,
                 from vanilla.cpp:22:
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note: void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = QuantLib::Date; _Alloc = std::allocator<QuantLib::Date>; std::vector<_Tp, _Alloc>::value_type = QuantLib::Date]
       push_back(const value_type& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note:   no known conversion for argument 1 from ‘boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type {aka boost::posix_time::ptime}’ to ‘const value_type& {aka const QuantLib::Date&}’
vanilla.cpp: In function ‘Rcpp::List americanOptionEngine(std::string, double, double, double, double, double, double, int, int, std::string, std::vector<double>, std::vector<double>)’:
vanilla.cpp:175:90: error: no matching function for call to ‘std::vector<QuantLib::Date>::push_back(boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type)’
             discreteDividendDates.push_back(today.dateTime() + discreteDividendLengths[i]);
                                                                                          ^
vanilla.cpp:175:90: note: candidate is:
In file included from /usr/include/c++/4.8/vector:64:0,
                 from /usr/include/boost/detail/container_fwd.hpp:93,
                 from /usr/include/boost/functional/hash/extensions.hpp:17,
                 from /usr/include/boost/functional/hash/hash.hpp:540,
                 from /usr/include/boost/functional/hash.hpp:6,
                 from /usr/include/boost/unordered/unordered_set.hpp:20,
                 from /usr/include/boost/unordered_set.hpp:16,
                 from /usr/include/ql/patterns/observable.hpp:37,
                 from /usr/include/ql/event.hpp:29,
                 from /usr/include/ql/cashflow.hpp:28,
                 from /usr/include/ql/quantlib.hpp:10,
                 from ../inst/include/rquantlib_internal.h:24,
                 from ./rquantlib.h:33,
                 from vanilla.cpp:22:
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note: void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = QuantLib::Date; _Alloc = std::allocator<QuantLib::Date>; std::vector<_Tp, _Alloc>::value_type = QuantLib::Date]
       push_back(const value_type& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:901:7: note:   no known conversion for argument 1 from ‘boost::date_time::base_time<boost::posix_time::ptime, boost::date_time::counted_time_system<boost::date_time::counted_time_rep<boost::posix_time::millisec_posix_time_system_config> > >::time_type {aka boost::posix_time::ptime}’ to ‘const value_type& {aka const QuantLib::Date&}’
make: *** [vanilla.o] Error 1
ERROR: compilation failed for package ‘RQuantLib’
* removing ‘/home/travis/build/eddelbuettel/rquantlib/RQuantLib.Rcheck/RQuantLib’
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

You probably need one more cast here. Should be simple enough. I'll try to take a look on the train ride in.

Owner

eddelbuettel commented Oct 24, 2016

You probably need one more cast here. Should be simple enough. I'll try to take a look on the train ride in.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

That's in the 'have TQL 1.7.* or newer and intra-day dates' branch. I didn't get it fixed on the train [1] but I'll have something for you hopefully this evening.

That whole section needs some work though. You know your vector sizes, so you can allocate at once rather than push_back each. Also the 'inside the loop' additional vectors are unneeded. We'll get there...

[1] I am currently making Date + Datetime vectors are little nice in Rcpp and had to roll that to the release version too.

Owner

eddelbuettel commented Oct 24, 2016

That's in the 'have TQL 1.7.* or newer and intra-day dates' branch. I didn't get it fixed on the train [1] but I'll have something for you hopefully this evening.

That whole section needs some work though. You know your vector sizes, so you can allocate at once rather than push_back each. Also the 'inside the loop' additional vectors are unneeded. We'll get there...

[1] I am currently making Date + Datetime vectors are little nice in Rcpp and had to roll that to the release version too.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 24, 2016

Contributor

Thanks! For some reason it wasn't compiling when I was skipping the extra vector in the loop. I did warn you that my C++ was very limited :) But hopefully it will get better.

Contributor

hfty commented Oct 24, 2016

Thanks! For some reason it wasn't compiling when I was skipping the extra vector in the loop. I did warn you that my C++ was very limited :) But hopefully it will get better.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

Can you do me a quick favor and send me a link (maybe into the QL repo at GH) about the example you used to guide your implementation. The time duration hack with the minutes() looks like something we can improve on. These time difference will also (marginally) impact the test comparison precision.

Owner

eddelbuettel commented Oct 24, 2016

Can you do me a quick favor and send me a link (maybe into the QL repo at GH) about the example you used to guide your implementation. The time duration hack with the minutes() looks like something we can improve on. These time difference will also (marginally) impact the test comparison precision.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 24, 2016

Contributor

This is the test unit, see DividendOptionTest::testEuropeanKnownValue().

(Edited: Added line numbers)

Contributor

hfty commented Oct 24, 2016

This is the test unit, see DividendOptionTest::testEuropeanKnownValue().

(Edited: Added line numbers)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

Don't rush things. This is still wrong. Every i-th step of the loop accesses only element i, after the loop the vector is done --- no need for a vector. Just do

boost::posix_time::time_duration discreteDividendLengths;

and use it as a temp. There are more things to fix here, and the change did not even try to address the real elephant in the room: your compilation failure.

Owner

eddelbuettel commented Oct 24, 2016

Don't rush things. This is still wrong. Every i-th step of the loop accesses only element i, after the loop the vector is done --- no need for a vector. Just do

boost::posix_time::time_duration discreteDividendLengths;

and use it as a temp. There are more things to fix here, and the change did not even try to address the real elephant in the room: your compilation failure.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 24, 2016

Contributor

Sorry, I was trying to makes things better, not worse! I'm not getting the compilation failure on my machine, so it is hard for me to figure that one out.

Contributor

hfty commented Oct 24, 2016

Sorry, I was trying to makes things better, not worse! I'm not getting the compilation failure on my machine, so it is hard for me to figure that one out.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

I already told you why (albeit in passing): you are running a QL variant not setup for intra-day times.

What OS are you on? There are some ways to correct for that (and some are easier than others).

Owner

eddelbuettel commented Oct 24, 2016

I already told you why (albeit in passing): you are running a QL variant not setup for intra-day times.

What OS are you on? There are some ways to correct for that (and some are easier than others).

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 24, 2016

Contributor

Yes, I got that it was the reason. I'm on macOS Sierra, using homebrew. I tried to install the --HEAD version, but the recipe fails, and I've not yet figured out why. I'm going to try to install from source.

Contributor

hfty commented Oct 24, 2016

Yes, I got that it was the reason. I'm on macOS Sierra, using homebrew. I tried to install the --HEAD version, but the recipe fails, and I've not yet figured out why. I'm going to try to install from source.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 24, 2016

Owner

Kewl. If and when you install from source make sure you add the required switch, approximately

tar xfz Quantlib-*tar.gz
cd QuantLib-*
./configure --enable-intraday
make
make install
Owner

eddelbuettel commented Oct 24, 2016

Kewl. If and when you install from source make sure you add the required switch, approximately

tar xfz Quantlib-*tar.gz
cd QuantLib-*
./configure --enable-intraday
make
make install
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 25, 2016

Owner

Ok. Try these halfes for the two #ifdef sets -- they work for me. May still be easiest if you commit that to get your PR in:

European exercise case

#ifdef QL_HIGH_RESOLUTION_DATE
    QuantLib::Date exDate(today.dateTime() + length);

    if(discreteDividends[0] != 0) {
        boost::posix_time::time_duration discreteDividendLength;        
        for (int i = 0; i < n; i++) {
            discreteDividendLength = boost::posix_time::minutes(discreteDividendsTimeUntil[i] * 360 * 24 * 60);
            QuantLib::Date dt(today.dateTime() + discreteDividendLength);
            discreteDividendDates.push_back(dt);
        }
    }
#else

American exercise case

#ifdef QL_HIGH_RESOLUTION_DATE
    QuantLib::Date exDate(today.dateTime() + length);

    if(discreteDividends[0] != 0) {
        boost::posix_time::time_duration discreteDividendLength;
        for (int i = 0; i < n; i++) {
            discreteDividendLength = boost::posix_time::minutes(discreteDividendsTimeUntil[i] * 360 * 24 * 60);
            QuantLib::Date dt(today.dateTime() + discreteDividendLength);
            discreteDividendDates.push_back(dt);
        }
    }
#else
Owner

eddelbuettel commented Oct 25, 2016

Ok. Try these halfes for the two #ifdef sets -- they work for me. May still be easiest if you commit that to get your PR in:

European exercise case

#ifdef QL_HIGH_RESOLUTION_DATE
    QuantLib::Date exDate(today.dateTime() + length);

    if(discreteDividends[0] != 0) {
        boost::posix_time::time_duration discreteDividendLength;        
        for (int i = 0; i < n; i++) {
            discreteDividendLength = boost::posix_time::minutes(discreteDividendsTimeUntil[i] * 360 * 24 * 60);
            QuantLib::Date dt(today.dateTime() + discreteDividendLength);
            discreteDividendDates.push_back(dt);
        }
    }
#else

American exercise case

#ifdef QL_HIGH_RESOLUTION_DATE
    QuantLib::Date exDate(today.dateTime() + length);

    if(discreteDividends[0] != 0) {
        boost::posix_time::time_duration discreteDividendLength;
        for (int i = 0; i < n; i++) {
            discreteDividendLength = boost::posix_time::minutes(discreteDividendsTimeUntil[i] * 360 * 24 * 60);
            QuantLib::Date dt(today.dateTime() + discreteDividendLength);
            discreteDividendDates.push_back(dt);
        }
    }
#else
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 25, 2016

Owner

Will you be able to apply those two changes to repair the pull request?

Owner

eddelbuettel commented Oct 25, 2016

Will you be able to apply those two changes to repair the pull request?

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 25, 2016

Contributor

Sure, thank you. I'll do that after lunch, I'm teaching this morning.

Contributor

hfty commented Oct 25, 2016

Sure, thank you. I'll do that after lunch, I'm teaching this morning.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 25, 2016

Owner

Perfect, was just worried as I hadn't heard from you. Maybe we can square this this evening (North American time).

Owner

eddelbuettel commented Oct 25, 2016

Perfect, was just worried as I hadn't heard from you. Maybe we can square this this evening (North American time).

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 25, 2016

Contributor

Yeah, sorry. I spend some time on compiling quantlib from source last night, but I have some unusual errors that I still can't figure out. But that shouldn't stop me from including your changes!

Contributor

hfty commented Oct 25, 2016

Yeah, sorry. I spend some time on compiling quantlib from source last night, but I have some unusual errors that I still can't figure out. But that shouldn't stop me from including your changes!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 25, 2016

Owner

I can't help much with OS X. I am subscribed to the two main QL mailing list, and that topic (of woes on OS X) seems to be recurring. I don't read each and every post on the list though -- but maybe you will a nugget there or can ask.

On Linux things "just work".

Also, while I have you here, we tend to have contributors 'unmasked'. Can you please provide a name and email address?

Owner

eddelbuettel commented Oct 25, 2016

I can't help much with OS X. I am subscribed to the two main QL mailing list, and that topic (of woes on OS X) seems to be recurring. I don't read each and every post on the list though -- but maybe you will a nugget there or can ask.

On Linux things "just work".

Also, while I have you here, we tend to have contributors 'unmasked'. Can you please provide a name and email address?

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 25, 2016

Contributor

I did a (long overdue) clean install of macOS Sierra, and Quantlib has been compiling for the last hour, no more error, it seems. I'll test the changes as soon as that's done and submit an updated PR.

Contributor

hfty commented Oct 25, 2016

I did a (long overdue) clean install of macOS Sierra, and Quantlib has been compiling for the last hour, no more error, it seems. I'll test the changes as soon as that's done and submit an updated PR.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 25, 2016

Owner

Great. Also:

Can you please provide a name and email address?

Owner

eddelbuettel commented Oct 25, 2016

Great. Also:

Can you please provide a name and email address?

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 25, 2016

Contributor

Sure. I sent you an email about that on your Debian address.

Contributor

hfty commented Oct 25, 2016

Sure. I sent you an email about that on your Debian address.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

Now vanilla RQuantLib does not compile with the Quantlib I compiled from source... Bear with me while I figure that out.

Contributor

hfty commented Oct 26, 2016

Now vanilla RQuantLib does not compile with the Quantlib I compiled from source... Bear with me while I figure that out.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 26, 2016

Owner

If you show error messages I may be able to help you.

Owner

eddelbuettel commented Oct 26, 2016

If you show error messages I may be able to help you.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

Thanks. Here's a partial gist. I was using RStudio so the error messages have been truncated. I'm doing it again from the command line and will update the gist when it's done.

Unless you have a better suggestion, I might try to recompile quantlib not from the github repository, but from the latest release, and see if that makes a difference.

Contributor

hfty commented Oct 26, 2016

Thanks. Here's a partial gist. I was using RStudio so the error messages have been truncated. I'm doing it again from the command line and will update the gist when it's done.

Unless you have a better suggestion, I might try to recompile quantlib not from the github repository, but from the latest release, and see if that makes a difference.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 26, 2016

Owner

Ah, yes, I would recommend the releases. I maintain them for Debian and Ubuntu too -- never had an issue with snapshots. "Random" git syncs are not my favourite way to get software.

Owner

eddelbuettel commented Oct 26, 2016

Ah, yes, I would recommend the releases. I maintain them for Debian and Ubuntu too -- never had an issue with snapshots. "Random" git syncs are not my favourite way to get software.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

Sounds good. It took a good couple of hours to compile last time, so I'm going to launch it tonight and report back in the morning. If it still doesn't work, I'll just do it on a Linux VM.

Contributor

hfty commented Oct 26, 2016

Sounds good. It took a good couple of hours to compile last time, so I'm going to launch it tonight and report back in the morning. If it still doesn't work, I'll just do it on a Linux VM.

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

All checks passed! Do I need to fix anything else?

Contributor

hfty commented Oct 26, 2016

All checks passed! Do I need to fix anything else?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 26, 2016

Owner

Not right now. I had a really casual first look when I had your branch checked out.

I will do two things 'for polish':

  • We really want Dates in the function interface, not the reals. Let me deal with this as it is a little under flux right (as I am updating Rcpp here). I intend to make this call compatible.
  • I may also wrap Nullable around it, your current practice of length(0) is not that bad though.
  • I may do one or two C++ style things.

But what would always help are more real-world examples. Can you punch up something either from Haug, or from John Hull's book, or some other standard?

Owner

eddelbuettel commented Oct 26, 2016

Not right now. I had a really casual first look when I had your branch checked out.

I will do two things 'for polish':

  • We really want Dates in the function interface, not the reals. Let me deal with this as it is a little under flux right (as I am updating Rcpp here). I intend to make this call compatible.
  • I may also wrap Nullable around it, your current practice of length(0) is not that bad though.
  • I may do one or two C++ style things.

But what would always help are more real-world examples. Can you punch up something either from Haug, or from John Hull's book, or some other standard?

@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

For the dates, sure, but then won't that be inconsistent with the maturity date being in fractional years? Could you detect the type of maturity and branch accordingly?

I'll dig through some books and try to find more examples. I think tests/RQuantlib.R could use some cleaning up – it references americanoption.cpp and europeanoption.cpp which I'm guessing have been replaced by vanilla.cpp.

Contributor

hfty commented Oct 26, 2016

For the dates, sure, but then won't that be inconsistent with the maturity date being in fractional years? Could you detect the type of maturity and branch accordingly?

I'll dig through some books and try to find more examples. I think tests/RQuantlib.R could use some cleaning up – it references americanoption.cpp and europeanoption.cpp which I'm guessing have been replaced by vanilla.cpp.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 26, 2016

Owner

(Rcpp) Dates are internally all doubles so you can be as fractional as you want:

R> as.numeric(difftime( as.Date(ISOdatetime(2016,10,16,10,0,0)), as.Date(ISOdatetime(2016,10,12,14,0,0))))
[1] 4
R> as.numeric(difftime( ISOdatetime(2016,10,16,10,0,0), ISOdatetime(2016,10,12,14,0,0)))
[1] 3.83333
R> 

I am a bit surprised by the first answer---but it does not limit us. Rcpp and QuantLib dates are more fine grained:

R> class(Sys.Date())
[1] "Date"
R> storage.mode(Sys.Date())
[1] "double"
R> 

Example cleanups and extensions are always welcome too.

Owner

eddelbuettel commented Oct 26, 2016

(Rcpp) Dates are internally all doubles so you can be as fractional as you want:

R> as.numeric(difftime( as.Date(ISOdatetime(2016,10,16,10,0,0)), as.Date(ISOdatetime(2016,10,12,14,0,0))))
[1] 4
R> as.numeric(difftime( ISOdatetime(2016,10,16,10,0,0), ISOdatetime(2016,10,12,14,0,0)))
[1] 3.83333
R> 

I am a bit surprised by the first answer---but it does not limit us. Rcpp and QuantLib dates are more fine grained:

R> class(Sys.Date())
[1] "Date"
R> storage.mode(Sys.Date())
[1] "double"
R> 

Example cleanups and extensions are always welcome too.

@eddelbuettel eddelbuettel merged commit 624d2d3 into eddelbuettel:master Oct 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hfty

This comment has been minimized.

Show comment
Hide comment
@hfty

hfty Oct 26, 2016

Contributor

But then there's the question of the day counting convention. I guess we should move this discussion to the issue tracker, anyway. Thanks for the merge!

Contributor

hfty commented Oct 26, 2016

But then there's the question of the day counting convention. I guess we should move this discussion to the issue tracker, anyway. Thanks for the merge!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 26, 2016

Owner

Day count conventions are covered at length in QL -- which is why I like to follow their examples.

Ie the one you worked off just add 'two months' and 'five months'. That is pretty clean, and users can (as they should be able to) do that in R.

But yes, new issue for new examples?

Owner

eddelbuettel commented Oct 26, 2016

Day count conventions are covered at length in QL -- which is why I like to follow their examples.

Ie the one you worked off just add 'two months' and 'five months'. That is pretty clean, and users can (as they should be able to) do that in R.

But yes, new issue for new examples?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Oct 29, 2016

Owner

@hfty: Ok, I gave it one quick do-over using Nullable<> (which requires Rcpp types -- so we make one more copy but the code is cleaner) and did some minor reorg. It is still using "fractional" time. Have a look -- in this branch and now also PR #74 -- and let me know what you think.

It may still be interesting to try to use actual dates, But it doesn't fit the current paradigm so it needs some thought.

Owner

eddelbuettel commented Oct 29, 2016

@hfty: Ok, I gave it one quick do-over using Nullable<> (which requires Rcpp types -- so we make one more copy but the code is cleaner) and did some minor reorg. It is still using "fractional" time. Have a look -- in this branch and now also PR #74 -- and let me know what you think.

It may still be interesting to try to use actual dates, But it doesn't fit the current paradigm so it needs some thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment