Move to C++17

[shrit]

No description provided.

[rcurtin]

Nice, it will be good to get this merged in.

There are also a number of methods that were recently modified to have a huge pile of constructors and Train() methods, but with C++17 we can use std::optional to reduce the number of overloads. For instance: #3578, #3572. We should definitely do that as part of this PR. I think any method that has documentation in user/methods/ should be checked for this issue.

Also grepping through the documentation and source code for words like C++14 or C++17 (or even C++11...) might help uncover some other things that can be updated.

[shrit]

We should open another two issues for things we should do using C++17 features. I would prefer to merge this as it is and then we can think slowly of things to do later.

[rcurtin]

The issue is that then we have only half-transitioned to C++17, and then you have a codebase that is inconsistent (it's like having ifdefs for ancient Armadillo versions that are less than the required version). There are a handful of things that we wanted to do when we made this transition, so why not do them now? There's not a particular hurry to get this merged, and I don't think it will take that long anyway---the cleanups and refactorings should be pretty straightforward.

We also need to update the documentation where relevant to indicate that C++17 is the minimum required standard now.

[shrit]

@rcurtin this is ready for review.

[rcurtin]

Awesome, everything here is looking good to me. I wasn't sure why the tests failed, so I built locally and dug in a bit. The comments I left detail what's wrong, it actually turned out to be a pretty simple issue.

I also went through all the methods I had documented to find more where I had written a bunch of Train() methods because I couldn't use std::optional. Here are the others I found:

• AdaBoost
• LinearRegression
• LinearSVM
• NaiveBayesClassifier
• Perceptron

Happy to help out with the conversion process, just let me know. I think the new versions of the Train() functions could be simplified with the ternary operator; I left a comment with a suggestion.

The only other main thing on my list was documentation so that users know C++17 is required. I think these places need to be modified:

• HISTORY.md (just a changelog note should be good)
• section 2 of README.md (C++14 compiler -> C++17 compiler)
• The R bindings specify C++14, but we should bump that in src/mlpack/bindings/R/mlpack/R/inline.R, to settings$env$USE_CXX17 (I think that will work)
• The documentation for the R bindings in src/mlpack/bindings/R/mlpack/DESCRIPTION.in

That's just what I found by grepping, I think that should be everything but I'm not 100% sure.

[shrit]

@rcurtin This is done on my side.
NaiveBayesClassifier can not be refactored, looking at the functions each Train function is different from the others.

LinearSVM Train functions were more complicated the Callbacks... are at the end of the function parameters and are shadowing the function calls, so I was only able to remove two functions, anything else will trigger an error.

[conradsnicta]

@shrit What about detection of C++17 as mentioned in comment #3704 (comment) ?

[shrit]

@conradsnicta Yes I will add it, sorry, I forgot about it.
Not to mention that the tests are still failing

[rcurtin]

Nice, everything is looking good to me. Just a couple comments throughout, hopefully they are helpful.

I took a look at NaiveBayesClassifier, it seems like we should be able to combine the first two Train() overloads; did I overlook a reason why that doesn't work?

[rcurtin]

@eddelbuettel @coatless can you sanity check this change here? And are there any other things than this (and the update to DESCRIPTION.in) that need to be updated if we are migrating to C++17?

[eddelbuettel]

I think this is technically correct ... but inline is not something we use when we build the CRAN package it is simply a (very nice to have, now that we're header-only) convenience for users. We could just say "R (>= 4.3.0)" in DESCRIPTION. Either way works.

[coatless]

We probably should do both.

[rcurtin]

There is one more overload below with intercept, can we combine that here too?

[rcurtin]

As far as I can tell it is also possible to combine this with the version that has callbacks below; I tested it locally. The signature is this:

  template<typename MatType,
           typename... CallbackTypes,
           typename = typename std::enable_if<IsEnsCallbackTypes<
               CallbackTypes...
           >::value>::type>
  ElemType Train(const MatType& data,
                 const arma::Row<size_t>& labels,
                 const size_t numClasses,
                 const std::optional<double> lambda = std::nullopt,
                 const std::optional<double> delta = std::nullopt,
                 const std::optional<bool> fitIntercept = std::nullopt,
                 CallbackTypes&&... callbacks);

Did you encounter any problems when trying that? It could allow us to remove another overload.

[shrit]

Yes, most of the problems were in the tests, especially if you remove the overload with the optimiser parameter and the callbacks and then try using an optimizer and not adding callbacks then the compiler will not be able to find the correct one

[rcurtin]

When I tried the code above, I had no compilation issues in the tests. Do you have a particular test case that fails to compile? I can try and debug it.

[eddelbuettel]

R has no issues with C++17. It has been the default for the last two releases (conditional on a suitable compiler present). We should be able to remove all checks (at the minimal "cost" of no longer supporting ancient R 3.6.* or so).

The manual says that C++17 became the minimum with R 4.3.0 last year. C++17 as a standard is not uncommon at CRAN. (A few packages use C++20, that is much dicier because of compiler support).

The package I look after for $work has been C++17 for some time now (and our internal code [which the R package may at time consume via pre-made library blobs we make available] uses C++20 but we keep the API at C++17).

[rcurtin]

Nearly there, I think from my side the only thing that remains is #3704 (comment) 👍

(I requested changes just to prevent any accidental merge before handling that)

[shrit]

Well the linear regression tests are failing, and GDB is not of a greater help 😄 , I need to compile with debug option to see what is really happening

[rcurtin]

I pushed a change that should fix the build; the compiler was using the deprecated version for some LinearRegression::Train() calls due to implicit conversions, so I made the deprecated versions "explicit" by using SFINAE to disable implicit conversions. It's an ugly workaround for the lack of explicit keyword for arbitrary member functions...

[rcurtin]

Assuming the build passes, everything looks good to me. 👍