thirty360 daycounter shouldn't be deprecated

[klin333]

Hi,

In commit 9c05688 utils.cpp was changed to remove support for QuantLib::Thirty360(), with comment saying it was deprecated as of QuantLib 1.23.

I don't believe that's true. In the release notes of QuantLib 1.23, it says: "Deprecated default constructor for actual/actual and 30/360 day counters; the desired convention should now be passed explicitly."

That is, the QuantLib just enforces a convention to be explicitly specified for thirty360. It did not remove support for thirty360.

Potential Fix

# utils.cpp Line 381
else if (n==6)
        return QuantLib::Thirty360(QuantLib::Thirty360::BondBasis);

Or, do what's done with the ActualActual, and add in more if else statements for different Thirty360 conventions. We probably should update Enum as well if we do that.

Thoughts?

[eddelbuettel]

Thanks for filing the issue. Let me take a quick look at what my QuantLib repo here has -- but in general I only 'retire' something in RQuantLib if it is no longer present in QuantLib. That said, I could have crossed lines, or made a bad adjustment. Which is what you are hinting add here....

Ok, took a look, and I think you are correct. I will fold your change in (once the current R CMD check passes).

[eddelbuettel]

Yep, that will go in. Thanks for spotting the error, and laying it out so cleanly in this issue.

[klin333]

Thanks for the quick response. After looking into this further, I'm more in favour of adding in additional if else statements for the different conventions of thirty360. This is because looking around Bloomberg, it appears Bloomberg differentiates between these conventions.

I've made an attempt below. Note that I've removed the #ifdef and chose a reasonable default for back compatibility. Ideally I should choose the convention that matches previous behaviour prior to interface change in QuantLib, however I'm not good enough with c++ and QuantLib to figure out what that is.

Unfortunately this code will likely need to change again soon because the development version of QuantLib appears to have added more conventions.

Also note that I've added an explicit error at the end. I think this is very important because the previous behaviour of silently defaulting to the arbitrary last else statement can easily conceal nasty bugs for the user since no-one will think of writing a test to check say n = 7 actually got them Actual365NoLeap (it wouldn't have, just a silent default to the last else case).

QuantLib::DayCounter getDayCounter(const double n){
    if (n==0)
        return QuantLib::Actual360();
    else if (n==1)
        return QuantLib::Actual365Fixed();
    else if (n==2)
        return QuantLib::ActualActual(QuantLib::ActualActual::ISDA); // reasonable default for back compatibility
    else if (n==3)
        return QuantLib::Business252();
    else if (n==4)
        return QuantLib::OneDayCounter();
    else if (n==5)
        return QuantLib::SimpleDayCounter();
    else if (n==6)
        return QuantLib::Thirty360(QuantLib::Thirty360::BondBasis);  // reasonable default for back compatibility
#ifdef RQUANTLIB_USE_ACTUAL365NOLEAP
     else if (n==7)
         return QuantLib::Actual365NoLeap();
#endif
    else if (n==8)
        return QuantLib::ActualActual(QuantLib::ActualActual::ISMA);
    else if (n==9)
        return QuantLib::ActualActual(QuantLib::ActualActual::Bond);
    else if (n==10)
        return QuantLib::ActualActual(QuantLib::ActualActual::ISDA);
    else if (n==11)
        return QuantLib::ActualActual(QuantLib::ActualActual::Historical);
    else if (n==12)
        return QuantLib::ActualActual(QuantLib::ActualActual::AFB);
    else if (n==13)
        return QuantLib::ActualActual(QuantLib::ActualActual::Euro);
    else if (n==14)
        return QuantLib::Thirty360(QuantLib::Thirty360::USA);
    else if (n==15)
        return QuantLib::Thirty360(QuantLib::Thirty360::BondBasis);
    else if (n==16)
        return QuantLib::Thirty360(QuantLib::Thirty360::European);
    else if (n==17)
        return QuantLib::Thirty360(QuantLib::Thirty360::EurobondBasis);
    else if (n==18)
        return QuantLib::Thirty360(QuantLib::Thirty360::Italian);
    else if (n==19)
        return QuantLib::Thirty360(QuantLib::Thirty360::German);
    else
        // Stop on verbose error.
        // Do not silently default to the arbitrarily last else statement because it can conceal bugs in user code.
        throw std::range_error("Unknown option " + std::to_string(n)); // is this the right way to cast double to string?
}

test_that("day count convention", {
  d1 <- as.Date('2020-01-01')
  d2 <- as.Date('2020-07-01')
  actual_num <- as.numeric(d2 - d1)
  actual_denom <- 366
  
  expect_equal(yearFraction(d1, d2, 0), actual_num/360)           # Actual360
  expect_equal(yearFraction(d1, d2, 1), actual_num/365)           # Actual365Fixed
  expect_equal(yearFraction(d1, d2, 2), actual_num/actual_denom)  # ActualActual
  expect_equal(yearFraction(d1, d2, 6), 0.5)                      # Thirty360
  expect_error(yearFraction(d1, d2, 7))                           # Actual365NoLeap
  
  expect_equal(yearFraction(d1, d2, 14), 0.5)                     # Thirty360
  expect_equal(yearFraction(d1, d2, 15), 0.5)                     # Thirty360
  expect_equal(yearFraction(d1, d2, 16), 0.5)                     # Thirty360
  expect_equal(yearFraction(d1, d2, 17), 0.5)                     # Thirty360
  expect_equal(yearFraction(d1, d2, 18), 0.5)                     # Thirty360
  expect_equal(yearFraction(d1, d2, 19), 0.5)                     # Thirty360
})

[eddelbuettel]

In favor, in principle, and I also thought about the added 'degree of freedom' that QuantLib::Thirty360::BondBasis gives us -- but I can't check right now (and I don't have Bbg access these days).

Did you by chance see any use of this in the QuantLib examples? I often follow then as they are vetted.

Also, it maybe be a little easier for all if we can work with patches and pull requests. You can even edit the source file in the browser at GitHub and it will fork for you and all that jazz. I am sure we can sort this out in the next few days.