Feature: add enable/disable macro for expression templates in reverse mode autodiff #1313
Conversation
Add no expression template support
demroz
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1313 +/- ##
========================================
Coverage 95.10% 95.10%
========================================
Files 796 797 +1
Lines 67115 67126 +11
========================================
+ Hits 63830 63841 +11
Misses 3285 3285 see 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@mborland @ckormanyos is there any chance I can get this looked at? I've been working on some gradient optimizers in the meantime. Should be ready soon-ish |
|
I will take a look tomorrow morning and leave comments |
|
|
||
| g++ -DBOOST_MATH_REVERSE_MODE_ET_OFF | ||
|
|
||
| The expression templated version of this code doesn't currently interact nicely with the Boost.Math special function implementations, and will throw compile time errors. Disabling expression templates will often fix this, however various special functions are implemented in a way that breaks the automatic differentiation chain for certain values. Complete special function support may be added in the future. |
There was a problem hiding this comment.
I think it would be useful to have a table here that lists the special functions that expression templates supports, rather than the user having to experiment on seeing what compiles.
There was a problem hiding this comment.
That's fair. Is there any way to add tests that aren't expected to compile without breaking the entire process? If not I can test locally
There was a problem hiding this comment.
Yes. In addition to run in the Jamfile you have the options of run-fail, compile, and compile-fail. Example is here: https://github.com/boostorg/math/blob/develop/test/Jamfile.v2#L1629.
In the build log it'll show failed-as-expected
| } | ||
|
|
||
| /** @brief | ||
| * subtraction float - rvar |
There was a problem hiding this comment.
Does the current testing include C++23's <stdfloat> types? If not I think they should be added. It has its own special rules like narrowing is a compilation error rather than a warning.
There was a problem hiding this comment.
The test_autodiff_reverse.hpp is essentially a slightly modified copy of the forward mode version. It has a macro that checks for __STDCPP_FLOAT_*_T__
here. I believe this is what youre asking about, I've never really worked with C++23's <stdfloat> types
There was a problem hiding this comment.
Yes, but you'll need to add this to the top:
#if __has_include(<stdfloat>)
# include <stdfloat>
#endif
Since the macros are defined inside that header instead of globally.
There was a problem hiding this comment.
es, but you'll need to add this to the top:
Do we still need to query __has_include itself?
#if defined(__has_include)
#if __has_include(<stdfloat>)
# include <stdfloat>
#endif
#endif
There was a problem hiding this comment.
No; we don't support any compiler that lacks __has_include.
| auto ldexp(const expression<RealType, DerivativeOrder, ARG> &arg, const int &i) | ||
| { | ||
| BOOST_MATH_STD_USING | ||
| return arg * pow(static_cast<RealType>(2.0), i); |
There was a problem hiding this comment.
Does the case of HUGE_VAL need to be handled here or does the expression template value naturally handle that?
There was a problem hiding this comment.
It naturally handles huge values correctly. I added an explicit test case for this though. Works as expected
| #ifndef BOOST_MATH_REVERSE_MODE_ET_OFF | ||
| #define BOOST_MATH_REVERSE_MODE_ET_ON | ||
| #endif | ||
|
|
There was a problem hiding this comment.
It might be worth adding a check here that there's no crazy configuration problems:
#if defined(BOOST_MATH_REVERSE_MODE_ET_OFF) && defined(BOOST_MATH_REVERSE_MODE_ET_ON)
# error "BOOST_MATH_REVERSE_MODE_ET_OFF and BOOST_MATH_REVERSE_MODE_ET_ON are mutually exclusive"
#endif
There was a problem hiding this comment.
So this commit also changed the default behavior. Instead of if not explicitly ET_OFF then ET_ON, to if not explicitly ET_ON then ET_OFF (via fall through). Should ET_ON be the default for performance reasons? Or should ET_OFF be the default since it has broader compatibility. I would say ET_ON if we tell the user what works with it, and how to turn it off (which are both already covered in docs or previous comments)
@ckormanyos and @jzmaddock in case you guys have an opinion on the matter.
There was a problem hiding this comment.
My fault. I didn't think too hard about about your comment and just copy and pasted that piece of code. I think ET should be enabled ON by default. At least from personal experience its what makes the most sense to me. If there is no objection from @ckormanyos and @jzmaddock I'll fix it in my next commit.
There was a problem hiding this comment.
I think ET should be enabled ON by default. At least from personal experience its what makes the most sense to me.
It also makes sense to me.
This feature adds a
BOOST_MATH_REVERSE_MODE_ET_ONandBOOST_MATH_REVERSE_MODE_ET_OFFmacro to enable/disable expression templates for the reverse mode autodiff library, and adds some documentation regarding this feature.This is also probably a good place to open a discussions about the future of the library. Adding special function support is most likely the next step, and I'd love some help and advice with that. I'm guessing
tgammaandcyl_bessel_jare good places to start.The other place that I'd like to take this library is adding some optimization routines like gradient descent, SGD, Adam, LBFGS, etc...