[WIP] Implement std::complex<stan::math::var>

[syclik]

Summary

This implements std::complex<stan::math::var>. We accomplished this by template specialization for enough of the implementation where it'll work.

std::complex<T> for non primitive types is explicitly in unspecified. We implemented as much as was necessary for the basic functions to pass. This may have to be revisited as different standard template library implementations may trigger seg faults. For more background information beyond this, see #123.

Note: the implementation is intentionally at stan/math/rev/scal/fun/std_complex.hpp even though the rest of the std overloads are in stan/math/rev/core. This uses functions that need to be instantiated first, so it made sense that it wasn't in the core directory.

Here's what we implemented:

1. constructor
2. operator=
3. var norm(complex<var>)
4. operator/
5. operator*
6. operator/=
7. operator*=
8. operator==
9. operator!=
10. int isinf(complex<var>)
11. int isnan(complex<var>)
12. var abs(complex<var>)
13. complex<var> conj(complex<var>)
14. complex<var> proj(complex<var>)

Tests

There are unit tests for all the operators for std::complex and tests for a lot of the functions. We test that the right number of stan::math::vars are on the stack after the different operations.

The tests are located in: test/unit/math/rev/scal/fun/std_complex_test.cpp and can be run:

./runTests.py test/unit/math/rev/scal/fun/std_complex_test.cpp

For tests, we tested everything in the spec: https://en.cppreference.com/w/cpp/numeric/complex
That's every member function and non-member function

@bgoodri: your original example compiles and runs!

Side Effects

I had to add the constexpr specifier to the constructor for stan::math::var to make it compile. For details, see: constexpr specifier. @bob-carpenter, that should be fine, right?

Checklist

• Math issue complex numbers with vars #123.

• Copyright holder:

  • Generable (@syclik's code)
  • Trustees of Columbia University (@bbbales2's code)
  • Trustees of Columbia University (@bgoodri's code)

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[syclik]

@bbbales2 and @bgoodri, can you add your copyright info. @bgoodri, we used the tests you implemented on your branch.

[syclik]

@bob-carpenter, I'm hoping the review takes under half an hour. The implementation of std::complex<stan::math::var> isn't that large.

[bgoodri]

I had a lot of additional tests on the other branch

[syclik]

I can't get the doxygen to work! Can anyone help? I see this error message (almost immediately):

$ make doxygen 
mkdir -p doc/api
doxygen doxygen/doxygen.cfg
stan/math/rev/scal/fun/std_complex.hpp:43: error: documented symbol `constexpr std::complex< stan::math::var >::complex' was not declared or defined. (warning treated as error, aborting now)
make: *** [doxygen] Error 1

[syclik]

@bgoodri, what branch? I can try to move more of them over.

[bbbales2]

I'm hoping the review takes under half an hour.

Come on man, don't jinx this lol

@bgoodri put them in the new_complex_var branch

grep -m1 -r "complex" test/unit to find them

test/unit/math/rev/scal/fun/acosh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/sinh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/asin_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/proj_test.cpp:// https://en.cppreference.com/w/cpp/numeric/complex/proj
test/unit/math/rev/scal/fun/sqrt_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/norm_test.cpp:  std::complex<stan::math::var> z = std::complex<stan::math::var>(3, 4);
test/unit/math/rev/scal/fun/conj_test.cpp:  std::complex<stan::math::var> z(1, 2);
test/unit/math/rev/scal/fun/arg_test.cpp:  std::complex<stan::math::var> z
test/unit/math/rev/scal/fun/cos_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/abs_test.cpp:TEST(AgradRev, abs_complex) {
test/unit/math/rev/scal/fun/log_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/atan_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/pow_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/tanh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/asinh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/acos_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/sin_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/exp_test.cpp:  std::complex<stan::math::var> z
test/unit/math/rev/scal/fun/cosh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/polar_test.cpp:  std::complex<stan::math::var> z = std::polar(1, 0);
test/unit/math/rev/scal/fun/atanh_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/tan_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/scal/fun/log10_test.cpp:TEST(AgradRev, complex) {
test/unit/math/rev/core/var_test.cpp:TEST_F(AgradRev, complexNotNullIssue123) {
test/unit/math/fwd/scal/fun/abs_test.cpp:TEST(AgradFwdAbs, complexNotNullIssue123) {

Probly best to compare against a pull from new_complex_var to not miss anything.

I can do this if neither of you have started.

[syclik]

Ha. Maybe I should have used an hour resolution. What I really mean is that if the review takes half a day, then we didn't write the code and PR clearly enough. And yes, please start. It looks like we got most, but not all.

…

On Thu, Sep 13, 2018 at 1:21 AM Ben Bales ***@***.***> wrote: I'm hoping the review takes under half an hour. Come on man, don't jinx this lol @bgoodri <https://github.com/bgoodri> put them in the new_complex_var branch grep -m1 -r "complex" test/unit to find them test/unit/math/rev/scal/fun/acosh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/sinh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/asin_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/proj_test.cpp:// https://en.cppreference.com/w/cpp/numeric/complex/proj test/unit/math/rev/scal/fun/sqrt_test.cpp:TEST(AgradRev <https://en.cppreference.com/w/cpp/numeric/complex/projtest/unit/math/rev/scal/fun/sqrt_test.cpp:TEST(AgradRev>, complex) { test/unit/math/rev/scal/fun/norm_test.cpp: std::complex<stan::math::var> z = std::complex<stan::math::var>(3, 4); test/unit/math/rev/scal/fun/conj_test.cpp: std::complex<stan::math::var> z(1, 2); test/unit/math/rev/scal/fun/arg_test.cpp: std::complex<stan::math::var> z test/unit/math/rev/scal/fun/cos_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/abs_test.cpp:TEST(AgradRev, abs_complex) { test/unit/math/rev/scal/fun/log_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/atan_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/pow_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/tanh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/asinh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/acos_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/sin_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/exp_test.cpp: std::complex<stan::math::var> z test/unit/math/rev/scal/fun/cosh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/polar_test.cpp: std::complex<stan::math::var> z = std::polar(1, 0); test/unit/math/rev/scal/fun/atanh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/tan_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/log10_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/core/var_test.cpp:TEST_F(AgradRev, complexNotNullIssue123) { test/unit/math/fwd/scal/fun/abs_test.cpp:TEST(AgradFwdAbs, complexNotNullIssue123) { Probly best to compare against a pull from new_complex_var to not miss anything. I can do this if neither of you have started. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F6MGrpy2OQpKrzCVWK0_jMRQUgiiks5uaetsgaJpZM4Wmiy3> .

[bgoodri]

There are also some under test/unit/math/{prim,rev}/mat/ that are not found via grepping for "complex" because the complex numbers are created internally by Eigen.

…

On Thu, Sep 13, 2018 at 1:21 AM Ben Bales ***@***.***> wrote: I'm hoping the review takes under half an hour. Come on man, don't jinx this lol @bgoodri <https://github.com/bgoodri> put them in the new_complex_var branch grep -m1 -r "complex" test/unit to find them test/unit/math/rev/scal/fun/acosh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/sinh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/asin_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/proj_test.cpp:// https://en.cppreference.com/w/cpp/numeric/complex/proj test/unit/math/rev/scal/fun/sqrt_test.cpp:TEST(AgradRev <https://en.cppreference.com/w/cpp/numeric/complex/projtest/unit/math/rev/scal/fun/sqrt_test.cpp:TEST(AgradRev>, complex) { test/unit/math/rev/scal/fun/norm_test.cpp: std::complex<stan::math::var> z = std::complex<stan::math::var>(3, 4); test/unit/math/rev/scal/fun/conj_test.cpp: std::complex<stan::math::var> z(1, 2); test/unit/math/rev/scal/fun/arg_test.cpp: std::complex<stan::math::var> z test/unit/math/rev/scal/fun/cos_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/abs_test.cpp:TEST(AgradRev, abs_complex) { test/unit/math/rev/scal/fun/log_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/atan_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/pow_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/tanh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/asinh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/acos_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/sin_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/exp_test.cpp: std::complex<stan::math::var> z test/unit/math/rev/scal/fun/cosh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/polar_test.cpp: std::complex<stan::math::var> z = std::polar(1, 0); test/unit/math/rev/scal/fun/atanh_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/tan_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/scal/fun/log10_test.cpp:TEST(AgradRev, complex) { test/unit/math/rev/core/var_test.cpp:TEST_F(AgradRev, complexNotNullIssue123) { test/unit/math/fwd/scal/fun/abs_test.cpp:TEST(AgradFwdAbs, complexNotNullIssue123) { Probly best to compare against a pull from new_complex_var to not miss anything. I can do this if neither of you have started. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrqjKWOQuk01aYLs6rX2-N03URFPZpks5uaetsgaJpZM4Wmiy3> .

[bbbales2]

You both should sleep more. I'm working on moving the complex tests over now. Had to add a sqrt. I'll release my lock on the code and push what I have up before group meeting.

[bbbales2]

Alright commit says I moved all the tests over but I really didn't.

I did not move stuff from:

test/unit/math/fwd/scal/fun/abs_test.cpp
test/unit/math/mix/mat/functor/autodiff_test.cpp
test/unit/math/rev/core/var_test.cpp
test/unit/math/rev/mat/fun/complex_eigenvalues_test.cpp
test/unit/math/rev/mat/fun/complex_schur_test.cpp
test/unit/math/rev/mat/fun/pseudo_eigendecomposition_test.cpp 

sqrt and pow on their own were causing segfaults.

The sqrt algorithm seems pretty complicated. I tried just coding up a textbook one and it didn't seem to behave well numerically. I just used the version that was segfaulting on my computer (removing the segfaulting bits) to write this and it seems to work.

I did not confirm that the things marked as failing in gcc-4.9 passed gcc-4.9. That is exp_test and pow_test.

[syclik]

So more segfaults? That's something. We can figure out what's causing it. Did you check those tests in? Mind pointing me to them?

…

On Thu, Sep 13, 2018 at 8:36 AM Ben Bales ***@***.***> wrote: Alright commit says I moved all the tests over but I really didn't. I did not move stuff from: test/unit/math/fwd/scal/fun/abs_test.cpp test/unit/math/mix/mat/functor/autodiff_test.cpp test/unit/math/rev/core/var_test.cpp test/unit/math/rev/mat/fun/complex_eigenvalues_test.cpp test/unit/math/rev/mat/fun/complex_schur_test.cpp test/unit/math/rev/mat/fun/pseudo_eigendecomposition_test.cpp sqrt and pow on their own were causing segfaults. The sqrt algorithm seems pretty complicated. I tried just coding up a textbook one and it didn't seem to behave well numerically. I just used the version that was segfaulting on my computer (removing the segfaulting bits) to write this and it seems to work. I did not confirm that the things marked as failing in gcc-4.9 passed gcc-4.9. That is exp_test and pow_test. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F64DOJsJhCnH5HA4531qCHQGnpypks5ualE2gaJpZM4Wmiy3> .

[bbbales2]

Implementing sqrt and pow fixed them.

They were more comparisons against __Tp()

[syclik]

Fun. I can't compile std::exp(std::complex<stan::math::var>)

$ ./runTests.py test/unit/math/rev/scal/fun/exp_test.cpp
------------------------------------------------------------
make -j1 test/unit/math/rev/scal/fun/exp_test
clang++ -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/sundials_3.1.0/include -std=c++1y -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION -Wno-unused-function -Wno-uninitialized -Wno-unknown-warning-option -Wno-tautological-compare -Wsign-compare -DGTEST_USE_OWN_TR1_TUPLE -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe  -c -o test/unit/math/rev/scal/fun/exp_test.o test/unit/math/rev/scal/fun/exp_test.cpp
In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/core.hpp:5:
In file included from ./stan/math/rev/core/build_vari_array.hpp:4:
In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4:
In file included from lib/eigen_3.3.3/Eigen/Dense:1:
In file included from lib/eigen_3.3.3/Eigen/Core:80:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:246:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:586:12: error: no matching
      function for call to 'isinf'
    return isinf(__lcpp_x);
           ^~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1063:9: note: in
      instantiation of function template specialization 'std::__1::__libcpp_isinf_or_builtin<stan::math::var>' requested here
    if (__libcpp_isinf_or_builtin(__x.real()))
        ^
./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization
      'std::__1::exp<stan::math::var>' requested here
                    : std::exp(y * std::log(x));
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:473:1: note: candidate
      template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var]
isinf(_A1 __lcpp_x) _NOEXCEPT
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:483:1: note: candidate
      template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var]
isinf(_A1) _NOEXCEPT
^
In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/core.hpp:5:
In file included from ./stan/math/rev/core/build_vari_array.hpp:4:
In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4:
In file included from lib/eigen_3.3.3/Eigen/Dense:1:
In file included from lib/eigen_3.3.3/Eigen/Core:80:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:246:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:606:12: error: no matching
      function for call to 'isfinite'
    return isfinite(__lcpp_x);
           ^~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1067:18: note: in
      instantiation of function template specialization 'std::__1::__libcpp_isfinite_or_builtin<stan::math::var>' requested here
            if (!__libcpp_isfinite_or_builtin(__i))
                 ^
./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization
      'std::__1::exp<stan::math::var>' requested here
                    : std::exp(y * std::log(x));
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:439:1: note: candidate
      template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var]
isfinite(_A1 __lcpp_x) _NOEXCEPT
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:449:1: note: candidate
      template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var]
isfinite(_A1) _NOEXCEPT
^
In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/core.hpp:5:
In file included from ./stan/math/rev/core/build_vari_array.hpp:4:
In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4:
In file included from lib/eigen_3.3.3/Eigen/Dense:1:
In file included from lib/eigen_3.3.3/Eigen/Core:80:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1070:31: error: no matching
      function for call to '__libcpp_isfinite_or_builtin'
        else if (__i == 0 || !__libcpp_isfinite_or_builtin(__i))
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization
      'std::__1::exp<stan::math::var>' requested here
                    : std::exp(y * std::log(x));
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:592:1: note: candidate
      template ignored: requirement 'is_floating_point<var>::value' was not satisfied [with _A1 = stan::math::var]
__libcpp_isfinite_or_builtin(_A1 __lcpp_x) _NOEXCEPT
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:604:1: note: candidate
      template ignored: substitution failure [with _A1 = stan::math::var]
__libcpp_isfinite_or_builtin(_A1 __lcpp_x) _NOEXCEPT
^
3 errors generated.
make: *** [test/unit/math/rev/scal/fun/exp_test.o] Error 1
make -j1 test/unit/math/rev/scal/fun/exp_test failed
exit now (09/13/18 10:23:41 EDT)

[syclik]

that was just a note to self.

[bgoodri]

Yeah, I remember we had to do something to work around the finiteness functions for some compilers on the other branch.

…

On Thu, Sep 13, 2018 at 10:25 AM Daniel Lee ***@***.***> wrote: Fun. I can't compile std::exp(std::complex<stan::math::var>) $ ./runTests.py test/unit/math/rev/scal/fun/exp_test.cpp ------------------------------------------------------------ make -j1 test/unit/math/rev/scal/fun/exp_test clang++ -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/sundials_3.1.0/include -std=c++1y -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION -Wno-unused-function -Wno-uninitialized -Wno-unknown-warning-option -Wno-tautological-compare -Wsign-compare -DGTEST_USE_OWN_TR1_TUPLE -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe -c -o test/unit/math/rev/scal/fun/exp_test.o test/unit/math/rev/scal/fun/exp_test.cpp In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1: In file included from ./stan/math/rev/scal.hpp:4: In file included from ./stan/math/rev/core.hpp:5: In file included from ./stan/math/rev/core/build_vari_array.hpp:4: In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4: In file included from lib/eigen_3.3.3/Eigen/Dense:1: In file included from lib/eigen_3.3.3/Eigen/Core:80: In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:246: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:586:12: error: no matching function for call to 'isinf' return isinf(__lcpp_x); ^~~~~ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1063:9: note: in instantiation of function template specialization 'std::__1::__libcpp_isinf_or_builtin<stan::math::var>' requested here if (__libcpp_isinf_or_builtin(__x.real())) ^ ./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization 'std::__1::exp<stan::math::var>' requested here : std::exp(y * std::log(x)); ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:473:1: note: candidate template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var] isinf(_A1 __lcpp_x) _NOEXCEPT ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:483:1: note: candidate template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var] isinf(_A1) _NOEXCEPT ^ In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1: In file included from ./stan/math/rev/scal.hpp:4: In file included from ./stan/math/rev/core.hpp:5: In file included from ./stan/math/rev/core/build_vari_array.hpp:4: In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4: In file included from lib/eigen_3.3.3/Eigen/Dense:1: In file included from lib/eigen_3.3.3/Eigen/Core:80: In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:246: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:606:12: error: no matching function for call to 'isfinite' return isfinite(__lcpp_x); ^~~~~~~~ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1067:18: note: in instantiation of function template specialization 'std::__1::__libcpp_isfinite_or_builtin<stan::math::var>' requested here if (!__libcpp_isfinite_or_builtin(__i)) ^ ./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization 'std::__1::exp<stan::math::var>' requested here : std::exp(y * std::log(x)); ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:439:1: note: candidate template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var] isfinite(_A1 __lcpp_x) _NOEXCEPT ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:449:1: note: candidate template ignored: requirement 'std::is_arithmetic<var>::value' was not satisfied [with _A1 = stan::math::var] isfinite(_A1) _NOEXCEPT ^ In file included from test/unit/math/rev/scal/fun/exp_test.cpp:1: In file included from ./stan/math/rev/scal.hpp:4: In file included from ./stan/math/rev/core.hpp:5: In file included from ./stan/math/rev/core/build_vari_array.hpp:4: In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4: In file included from lib/eigen_3.3.3/Eigen/Dense:1: In file included from lib/eigen_3.3.3/Eigen/Core:80: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/complex:1070:31: error: no matching function for call to '__libcpp_isfinite_or_builtin' else if (__i == 0 || !__libcpp_isfinite_or_builtin(__i)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./stan/math/rev/scal/fun/std_complex.hpp:164:28: note: in instantiation of function template specialization 'std::__1::exp<stan::math::var>' requested here : std::exp(y * std::log(x)); ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:592:1: note: candidate template ignored: requirement 'is_floating_point<var>::value' was not satisfied [with _A1 = stan::math::var] __libcpp_isfinite_or_builtin(_A1 __lcpp_x) _NOEXCEPT ^ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:604:1: note: candidate template ignored: substitution failure [with _A1 = stan::math::var] __libcpp_isfinite_or_builtin(_A1 __lcpp_x) _NOEXCEPT ^ 3 errors generated. make: *** [test/unit/math/rev/scal/fun/exp_test.o] Error 1 make -j1 test/unit/math/rev/scal/fun/exp_test failed exit now (09/13/18 10:23:41 EDT) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrqu7eMLukvNpn7eR5d_AN4UOhtFZsks5uamrFgaJpZM4Wmiy3> .

[seantalts]

Did we use code from @ChrisChiasson's PR? Do we need his copyright release as well?

[syclik]

No, we did not. Everything was tested and written from scratch.

…

On Mon, Sep 17, 2018 at 1:29 PM seantalts ***@***.***> wrote: Did we use code from @ChrisChiasson <https://github.com/ChrisChiasson>'s PR? Do we need his copyright release as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F2xd3mDa291NkzJGqY8cQgoXrZt_ks5ub9vygaJpZM4Wmiy3> .

[syclik]

Thanks for your comments! More inline.

On Mon, Sep 17, 2018 at 4:43 PM ChrisChiasson ***@***.***> wrote: I am going to bring some things up now, since I wasn't aware of this PR until a couple of hours ago today, and because I want to avoid criticism for failing to bring these issues to light before a lot of time is expended on the PR, and because this PR's landing will affect my ability to continue to synchronize my patch & prevent my applications from bit rotting. I'm keeping this comment just to major end-user functionality.

Is your patch still used? The branch was deleted after the PR was closed. Anyway, this PR will move without taking that branch into account for the reason it was assumed it was removed.

- It seems like fvar isn't supported, possibly to make coding this PR easier? Without fvar, it is hard to do efficient 2nd derivative operations on algorithms... fvar support is tricky, because fvar can contain var, giving it the same initialization property requirements as var, but with the added pain of it also being a template.

That's right. This PR only deals with reverse mode. Forward mode would be handled by a different PR. This was to make sure we understood end-to-end behavior properly and what sorts of pitfalls we needed to deal with. Btw, as mentioned, I really believe your code to be very clever. But, I think it's unnecessarily complex. This PR implements a much simplier design doesn't need to add so much template metaprogramming. For forward mode, we could employ a similar strategy.

- I don't see code that handles parameter coercions. Here are links to the original tests (sorry, I'm like a broken record) for parameter conversions as well as the basic +,-,*,/ operation tests for both var <https://github.com/stan-dev/math/blob/07ffd6c0dde3ed919248a4d43cd80bba066ceeba/test/unit/math/rev/core/var_test.cpp#L91-L99> and fvar <https://github.com/stan-dev/math/blob/07ffd6c0dde3ed919248a4d43cd80bba066ceeba/test/unit/math/fwd/scal/fun/abs_test.cpp#L68-L76>. For example, with standard complex variables in C++, you can do: 5.0*std::complex(1.0,2.0), and this will give std::complex(5.0,10.0). In general, stan is designed to allow the same sorts of expressions that work with doubles to work with vars, so it's reasonable to support complexes of AD type without having to explicitly cast the 5.0--so that user code is more understandable. This is also relevant when using 3rd party templated code and feeding in various user types, some of which are AD complexes...

We didn't need to do the coercions you're talking about because of the way we implemented the type. See tests here: https://github.com/stan-dev/math/blob/fc218ea5907eb885a3c3c06853fd917f36e53221/test/unit/math/rev/scal/fun/std_complex_test.cpp#L87 If you think we missed a use case (which we might have!), please add a unit test that shows it.

- The same comment as above, but for scalars, vectors, and matrices of non-complexes multiplied with those of complexes possibly of AD type. Eigen::Matrix<double,R,C>()*Eigen::Matrix<complex<double>,C,R>() is allowed normally, so we should expect it to also work when the complex matrix is of some AD type while the non-complex remains just double, which the old patch also handles <https://github.com/stan-dev/math/blob/07ffd6c0dde3ed919248a4d43cd80bba066ceeba/stan/math/prim/mat/meta/Eigen_ScalarBinaryOpTraits.hpp#L15-L35> .

All of that should work with this implementation (for reverse mode, not forward mode). If you find a counterexample, please let us know with an example!

…

- — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F1B2EwcFbxiDg81fsUQ9-6KhBFORks5ucAllgaJpZM4Wmiy3> .

[syclik]

Once again, thanks for engaging. I really appreciate your input. You've thought about this stuff much more than I have.

Re: branch deleted?: Your copy of that branch wasn't deleted... I have linked to it three times in my previous post...

At the time you closed the PR, I didn't realize it was still on the repo. See the original PR. If your interface looks like mine, it should look like:

From the GitHub PR, it just looks like it disappeared. And you didn't respond to the last questions on the thread, so I assumed it was gone. (I don't know every branch that's on our GitHub repo.)

Re: parameter coercions: I had read the test you linked me to before I wrote my previous post. I don't see how it addresses my second bullet point. Your test parameters are already vars. My point is about one of the parameters not being a var. Maybe I am just too stupid to figure it out, but I don't see it.

Nope, you're absolutely right! The tests don't have that. But... the way the implementation is done, we don't need to implement "parameter coercion" in that way (I think I'd just call that operator=). That's part of the simplicity of this design: we actually get to use stan::math::var's operators.

That said, I'll test it and push a new test for that use case to verify. I don't believe myself until I see it in code.

Re: using eigen as-is: The eigen code I pointed out can't just be used as-is if the relevant operator coercions are not defined.

Ok. But I think we're covered. We'll see soon enough.

Re: fvars: I am raising the point about fvars now because I believe it will be challenging to use the type of technique you are trying to use here with it and maintain all the other properties I enumerated. Remember, I will be stuck implementing fvar on top of your code if you don't implement it.

Sorry about that. But... from the Stan development point of view, that won't have too much weight. That said, things don't move very fast, so no worries.

(least important part follows)
Re: old implementation 'too complex': Conversely, I have concerns about this 'simple' method containing essentially two extra copies of the complex implementation once fvar is included ... from a code surface area / maintainability / scope for bugs standpoint. The old implementation did reproduce some routines, but only the minimum required to work around standard library problems... To the maximum extent possible, fvar, var, and double itself were using the same code paths, lending more confidence to the implementation aside from just the testing. Lastly thinking about an apples to apples comparison, plenty of complexity entered the old implementation while I was trying to honor the layout of stan math and accomplish everything else at the same time... I think if your implementation were to actually do these things, it would be more complex than mine.

I hear your concerns, but I doubt it. It took me hours and hours to unpack what you did. It's super clever! And I see the merits of it. But it needs a lot more testing and thinking through how this actually affects everything else. With a simpler implementation, even though some of it would be duplicated, we're able to reason about it easier and it's easier to implement, maintain, and communicate.

That's all opinion. The things that really stopped your original PR was two-fold:

1. there was no good description of what you were trying to do enough to communicate to others. If it makes you feel any better, the core developers go through multiple iterations to get it right. I think where the issue is now, it's clear enough to communicate to others.
2. testing. The more core a feature / bugfix is, the more testing we require. There were some deep changes to the template metaprograms. Since there's so much risk, we'd want testing that reduced it. If there was enough, the design probably would have been let through (conditioned on having the appropriate doc too).

Anyway, take a look and see if you spot anything else. I was thinking about how to extend just this to work on all the autodiff (without needing to go through zeroing). I think it's doable, but first things first: getting this PR fully complete.

[syclik]

Sorry... I'm really not trying to be cute, adversarial, or anything. I linked to the PR directly. After it disappeared, I searched for half an hour to find the code and I couldn't. To me, it was gone. @bgoodri was able to recover the PR... you'll see that was a separate PR from this one. Anyway, sorry if you took offense to that. It wasn't intended and I'll try to think more carefully how I write things out. On Mon, Sep 17, 2018 at 7:54 PM ChrisChiasson <notifications@github.com> wrote:

…

I noticed how you have cropped that screenshot... There is plenty of UI at the top, middle, and bottom of the PR that makes it clear the code is still there, to say nothing of actually clicking to check. Besides, from where do you suppose all the tests on this branch were pulled? Some bizarro world copy of the old branch that just had the test files in it and nothing else? I will try to respond to the rest later or tomorrow. On Mon, Sep 17, 2018, 6:13 PM Daniel Lee ***@***.***> wrote: > Once again, thanks for engaging. I really appreciate your input. You've > thought about this stuff much more than I have. > > Re: branch deleted?: Your copy of that branch wasn't deleted... I have > linked to it three times in my previous post... > > At the time you closed the PR, I didn't realize it was still on the repo. > See the original PR <#789>. If your > interface looks like mine, it should look like: > > [image: image] > < https://user-images.githubusercontent.com/425751/45653051-8f841100-baa4-11e8-9b95-dc78aab385da.png > > > From the GitHub PR, it just looks like it disappeared. And you didn't > respond to the last questions on the thread, so I assumed it was gone. (I > don't know every branch that's on our GitHub repo.) > > Re: parameter coercions: I had read the test you linked me to before I > wrote my previous post. I don't see how it addresses my second bullet > point. Your test parameters are already vars. My point is about one of the > parameters not being a var. Maybe I am just too stupid to figure it out, > but I don't see it. > > Nope, you're absolutely right! The tests don't have that. But... the way > the implementation is done, we don't need to implement "parameter coercion" > in that way (I think I'd just call that operator=). That's part of the > simplicity of this design: we actually get to use stan::math::var's > operators. > > That said, I'll test it and push a new test for that use case to verify. I > don't believe myself until I see it in code. > > Re: using eigen as-is: The eigen code I pointed out can't just be used > as-is if the relevant operator coercions are not defined. > > Ok. But I think we're covered. We'll see soon enough. > > Re: fvars: I am raising the point about fvars now because I believe it > will be challenging to use the type of technique you are trying to use here > with it and maintain all the other properties I enumerated. *Remember, I > will be stuck implementing fvar on top of your code if you don't implement > it.* > > Sorry about that. But... from the Stan development point of view, that > won't have too much weight. That said, things don't move very fast, so no > worries. > > (*least* important part follows) > Re: old implementation 'too complex': Conversely, I have concerns about > this 'simple' method containing essentially two extra copies of the complex > implementation once fvar is included ... from a code surface area / > maintainability / scope for bugs standpoint. The old implementation did > reproduce some routines, but only the minimum required to work around > standard library problems... To the maximum extent possible, fvar, var, and > double itself were using the same code paths, lending more confidence to > the implementation aside from just the testing. Lastly thinking about an > apples to apples comparison, plenty of complexity entered the old > implementation while I was trying to honor the layout of stan math and > accomplish everything else at the same time... I think if your > implementation were to actually do these things, it would be more complex > than mine. > > I hear your concerns, but I doubt it. It took me hours and hours to unpack > what you did. It's super clever! And I see the merits of it. But it needs a > lot more testing and thinking through how this actually affects everything > else. With a simpler implementation, even though some of it would be > duplicated, we're able to reason about it easier and it's easier to > implement, maintain, and communicate. > > That's all opinion. The things that really stopped your original PR was > two-fold: > > 1. there was no good description of what you were trying to do enough > to communicate to others. If it makes you feel any better, the core > developers go through multiple iterations to get it right. I think where > the issue is now, it's clear enough to communicate to others. > 2. testing. The more core a feature / bugfix is, the more testing we > require. There were some deep changes to the template metaprograms. Since > there's so much risk, we'd want testing that reduced it. If there was > enough, the design probably would have been let through (conditioned on > having the appropriate doc too). > > Anyway, take a look and see if you spot anything else. I was thinking > about how to extend just this to work on all the autodiff (without needing > to go through zeroing). I think it's doable, but first things first: > getting this PR fully complete. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1031 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ALTv-zFR8ZIl4TKJXN129Nt1E6amSCxEks5ucCyJgaJpZM4Wmiy3 > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F_rCRfq3m_Q7y9QtDcrypWKjHxb0ks5ucDYugaJpZM4Wmiy3> .

[syclik]

@ChrisChiasson, you're absolutely right about operator* not having the right promotion rules! Ok... more thinking to do.

[syclik]

Yup. But now that I looked at your original PR again, I see the commits are embedded in there, just way down. (just above this comment: #789 (comment)). I have no idea why the commits are all bunched up there and not at the top or bottom. And now I don't recall if they were there in August. There's a chance they were always there and I just overlooked it somehow. Btw, if you're planning on removing all your comments again, mind letting us know. You're the first contributor that has done this regularly and it's tougher to engage knowing that things won't be around. It's just something we'll have to work around and it's better to know sooner rather than later.

…

On Mon, Sep 17, 2018 at 9:02 PM ChrisChiasson ***@***.***> wrote: >bgoodri was able to recover the PR... you'll see that was a separate PR from this one. Are you referring to this? #915 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_FyYWIQF9O-uzoV1k-sZefW_Q3gmHks5ucEYOgaJpZM4Wmiy3> .

[bob-carpenter]

.

On Sep 18, 2018, at 8:51 PM, ChrisChiasson ***@***.***> wrote: ... If you really want to know what process improvement I would make it is this:

Thanks for the constructive advice after all this. I'm really sorry this issue has gone down this way.

• Know yourself and how you use your power. Communicate it effectively to others. If you have a slight inkling that you will eventually do something like what you did here, just nip it in the bud early and tell the contributor that there is absolutely no way you will accept the code configured the way they want. Giving contributors reasons other than that only serves to make them believe that you are persuadable. However, after reasons shift under scrutiny, it gives way to a routine that can feel to contributors like a runaround, which is not a good way to attract and retain contributors.

I think this is the main issue we have. And we don't have agreement among the devs in charge of our repos, as to what that level is. But whatever we do, we need to communicate more clearly what the requirements. I can't promise to never change requirements as we go and learn things, but I don't want pull requests to be moving targets. I'm still not entirely sure what the goal is for this pull request or the proposed alternative. About all I know about complex numbers is that sqrt(-1) = i and that Eigen has a partial implementation for some operations.

• Hold your own code, and reasoning behind it, to the same level of scrutiny as you hold contributors' code.

Our goal is to hold everyone's code to the same standard. As a result, pull requestions from longstanding contributors have met with the same kind of pushback as in this issue and led to the same kind of aggravation all around. The only way to get around this is to make the steps clearer.

Be mindful of the barrier to entry being raised to outside contributors. ...

The reviewers are trying to balance barriers to entry with maintainability. At least I hope that's what they're doing. Not all of us agree on where the line should be, so this is going to require some ongoing negotiation to get consistency among our reviewers.

[bgoodri]

It would also probably be helpful if people would stop saying things like

I'm still not entirely sure what the goal is for this pull request or the proposed alternative.

It has been apparent to me for a long time that the goal is to extend Stan Math's autodiff mechanism to (complex) functions of the form x + i * y where i is a new constant whose square is -1 while x and y can be stan::math::var or stan::math::fvar<stan::math::var> or double. If that much is apparent to me, it should be apparent to everyone else, especially after I have said it five times.

However, the description of the "goal" has little to do with what is holding up this PR or the previous PR. The main questions are how to implement it and how to work around (mostly older) compiler and standard library limitations.

[bob-carpenter]

While I get the basic gist of

the goal is to extend Stan Math's autodiff mechanism to (complex) functions of the form x + i * y where i is a new constant whose square is -1 while x and y can be stan::math::var or stan::math::fvar<stan::math::var> or double.

I'm looking for more in the way of a functional spec that breaks this down into why we're doing this and what specifically we'll do.

The summary of the PR above is better. It does mention that it implements all the member and nonmember functions in the std::complex spec. But what about other things like numeric_limits specializations and traits specializations for Eigen? I don't see any mention of fvar. I also don't see which top-level header this will be included in, if any.

And what I meant about being too dense to see the ramifications, I still don't understand why anyone is interested in doing this. Is this purely a math library function for external applications or will it somehow connect to the Stan language at some point? Presumably just implementing complex<var> doesn't do anything in and of itself if there aren't other functions that operate on complex<var> types. As far as I know, we don't have any of those in the math library. I recall some discussion of some matrix operations that could operate with complex numbers---will this help with that? I recall other discussion about autodiff testing---will this help with that?

[bgoodri]

Briefly,

• The why is harder to convey because none of the things you would want to do are being implemented on this PR (or the previous one) but to do any of those things (FFTs, eigenvalues of asymmetric matrices, etc.) you would first need to implement the autodiff
• We basically need to implement what is in the C++ standard for complex<double> and what additional is "necessary" for Eigen but I don't believe numeric_limits is included in necessary
• We had stan::math::fvar<std::complex<stan::math::var> > working on the previous PR, but I don't think it is implemented yet on this PR. Perhaps, we could merge prim and rev stuff first and fwd stuff later.
• The Stan Math stuff isn't connected to the Stan language at all. In the future, possibly someone could do something like
  vector[K] lambda[2] = eigenvalues_asymmetric(X);
  to get the real and imaginary parts of the eigenvalues of the K by K asymmetric matrix X.
• If you just think in terms of C++14 and not about the Stan language, then a lot (but not all) functions in stan/math/{prim,rev}/{scal,mat,arr} do work for complex input.
• @bbbales2 and @yizhang-cae did want to use the complex step approximation for calculating derivatives. There are some tests of this on the previous branch. But basically for z = x + i * h where h = 10e-8 is the imaginary component, the derivative of foo(x) is given by imag(foo(z)) / h.

[bob-carpenter]

Yes, constexpr should be OK for var() because it just sets up a null pointer for the member vari.

Is this being used as a dummy value for initialization in std::complex? That should be OK because it'll be copied rather than shared by reference.

How is it used in std::complex? Is it something like a dummy value for initialization? If so, copies of it should be fine. But it can't be shared by reference if it's used for initialization.

should be OK here---it'll just create a null pointer version. The object isn't technically constant once created, though. What happens

[bob-carpenter]

Thanks, Ben. I think it's OK to do just reverse-mode first. Our ODE stuff only works for reverse mode. I'm not clear on why we'd want to do development in the math library that wasn't aimed at the Stan language or algorithms eventually. I don't want to get in the business of supporting a general toolkit just because we can. I can see how functions only using arithmetic would work, but I don't see how functions that call other functions like exp, log, or lgamma are going to work. And why were only some of the arithmetic operators implemented? Are there templated versions of += and so on that already work?

[syclik]

Regarding just some of the operators being template specialized: yes! We're relying on the base implementation and we don't need to implement them. We're testing them because implementations of STL differ and it's possible that it may work with clang++ and not g++ or vice versa. We've already run into a handful of problems like that. (We could implement the whole spec, but we're hoping that we could rely on the base implementation for a bulk of the work.

…

On Wed, Sep 19, 2018 at 8:12 AM Bob Carpenter ***@***.***> wrote: Thanks, Ben. I think it's OK to do just reverse-mode first. Our ODE stuff only works for reverse mode. I'm not clear on why we'd want to do development in the math library that wasn't aimed at the Stan language or algorithms eventually. I don't want to get in the business of supporting a general toolkit just because we can. I can see how functions only using arithmetic would work, but I don't see how functions that call other functions like exp, log, or lgamma are going to work. And why were only some of the arithmetic operators implemented? Are there templated versions of += and so on that already work? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F47IuPy5yIgtVDaa-twhEjLSPQvSks5ucjSbgaJpZM4Wmiy3> .

[ChrisChiasson]

Saw the recent code push. Any decisions made?

[syclik]

@ChrisChiasson, for reverse mode, we'll eventually go with this simpler design. This implementation isn't there yet; you were absolutely right. It missed some of the operators.

For forward and mixed mode, it's not clear. Your implementation might be the right thing to do. Or not. Haven't really evaluated any other designs.

Could you remind me what you actually use? Is it just reverse mode? Forward mode? Mixed?

[ChrisChiasson]

Mixed. For my initial use case [I have more now] see my old PR, which I'll restate works for mixed mode and actually tests mixed mode in a matrix environment... and passes the entire test suite aswell. I have also proposed a way around the inheritance structure that you didn't like. I can patch it into the old PR. I also fixed array coefficient wise operations on Stan types [not just complex Stan types] in my private copy. (Operations like matrix1.array() / matrix2.array() don't seem to work with Stan types without that.)

…

On Fri, Feb 1, 2019, 8:09 PM Daniel Lee ***@***.*** wrote: @ChrisChiasson <https://github.com/ChrisChiasson>, for reverse mode, we'll eventually go with this simpler design. This implementation isn't there yet; you were absolutely right. It missed some of the operators. For forward and mixed mode, it's not clear. Your implementation might be the right thing to do. Or not. Haven't really evaluated any other designs. Could you remind me what you actually use? Is it just reverse mode? Forward mode? Mixed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALTv-0suiZK6ut0cvOW8QFJ8ZydGU6VRks5vJPNpgaJpZM4Wmiy3> .

[syclik]

Cool. If you want that to be reviewed carefully, mind submitting another PR? Sorry -- memory's a little short. I'm juggling too much to remember everything (e.g. ode PRs and design, the autodiff testing framework, threading, MPI, design decisions, etc). Complex is lower priority for me now, but if you put up a PR that will work for the Math library (not just a one-time implementation, but it has to include design + maintainability), then I'd be happy to review it. On Fri, Feb 1, 2019 at 9:49 PM ChrisChiasson <notifications@github.com> wrote:

…

Mixed. For my initial use case [I have more now] see my old PR, which I'll restate works for mixed mode and actually tests mixed mode in a matrix environment... and passes the entire test suite aswell. I have also proposed a way around the inheritance structure that you didn't like. I can patch it into the old PR. I also fixed array coefficient wise operations on Stan types [not just complex Stan types] in my private copy. (Operations like matrix1.array() / matrix2.array() don't seem to work with Stan types without that.) On Fri, Feb 1, 2019, 8:09 PM Daniel Lee ***@***.*** wrote: > @ChrisChiasson <https://github.com/ChrisChiasson>, for reverse mode, > we'll eventually go with this simpler design. This implementation isn't > there yet; you were absolutely right. It missed some of the operators. > > For forward and mixed mode, it's not clear. Your implementation might be > the right thing to do. Or not. Haven't really evaluated any other designs. > > Could you remind me what you actually use? Is it just reverse mode? > Forward mode? Mixed? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1031 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ALTv-0suiZK6ut0cvOW8QFJ8ZydGU6VRks5vJPNpgaJpZM4Wmiy3 > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F1C6I68DGPiFA8W_EQPs5N8kPtsmks5vJPzQgaJpZM4Wmiy3> .

[ChrisChiasson]

Can you clarify
"will work for the Math library"?

Specifically, in your review of the old complex implementation was it, in your opinion
"just a one-time implementation"
that did not
"include design + maintainability"?

Setting aside how I personally feel about the wording itself and similar remarks previously, I find those comments to be... lacking in concrete justification.

I would like to know how the
"simpler design"
code on this PR actually does a better job at being able to
"work for the Math library".

Extra emphasis on "work".

These questions arise because my intended approach is to replace the inheritance trick in the old PR with the compromise I proposed, but leave everything aside from the inheritance trick the same.

Let's take one example - use of templates:
A fundamental tension seems to be that you oppose a lot of the auxiliary templates I added. You seem to prefer instead to have functions specifically written out with the explicit types. However, this puts anyone who wants to use fvar, Eigen, or even properly do a simple complex<var> * scalar in a difficult position, as we saw in our discussion above after the initial denials.

Consider this horribly clang-autoformatted Eigen code from the original PR, which I pointed out previously:

math/stan/math/prim/mat/meta/Eigen_ScalarBinaryOpTraits.hpp

Lines 16 to 34 in 07ffd6c

	template <class T1, class T2, template <class, class> class OP>
	struct ScalarBinaryOpTraits<
	T1,
	std::enable_if_t<
	// !VectorBlock !is_eigen
	(stan::is_complex<T1>::value || stan::is_arith_like<T1>::value)
	&& (stan::is_complex<T2>::value || stan::is_arith_like<T2>::value)
	&& !std::is_same<T1, T2>::value && // avoid Eigen's template
	((stan::is_complex<T1>::value && // next boolean avoids Eigen
	!std::is_same<stan::rm_complex_t<T1>,
	stan::rm_zeroing_t<T2>>::value)
	|| (stan::is_complex<T2>::value && // next bool avoids Eigen
	!std::is_same<stan::rm_zeroing_t<T1>,
	stan::rm_complex_t<T2>>::value)),
	T2>,
	OP<T1, T2>> {
	typedef std::complex<typename stan::return_type<stan::rm_complex_t<T1>,
	stan::rm_complex_t<T2>>::type>
	ReturnType;

This is stepping around a few template specializations in Eigen, triggering on the needed cases where Eigen's specializations won't, and not triggering when Eigen's will (thus avoiding compiler errors). As hard as this was to make it work and test it with the templates, it would be exponentially more difficult without them.

Also, templates make it easier to follow stan's structure of prim, fwd, rev, because the appropriate function templates are written and used once at the prim level, yet continue to work in the presence of the higher levels as they are included. This is the way templates are supposed to be used, and I thought stan was trying to move in that direction. It additionally provides far less code surface for errors.

If you're going to slow-walk the removal of the templates by throwing up a lot of barriers to them until I cave, there is basically no way I would have enough time or energy to complete the project as you envision it... Nor do I reasonably expect that anyone would, unless they were just trying to prove a point.

I'm trying to avoid the situation we ended up in earlier.

[syclik]

Closing until the implementation is ready to go.

[syclik]

@ChrisChiasson: I also don't want to get into a situation like that (and I never intended it to get there). I don't know if you recall, but the end result was that it could be merged if two things happened: 1) documentation, 2) unit tests.

I'll stick with my original assessment: please document the template meta programs and the rest of the additions according to Stan's documentation guidelines: [https://github.com/stan-dev/stan/wiki/How-to-Write-Doxygen-Doc-Comments] and write unit tests for the things you've added. If you want help with some of that work, there might be people willing to help on Stan's forums.

To create a PR, follow the guidelines we have: https://github.com/stan-dev/math/wiki/Developer-Doc#code-review-guidelines. It really would have really helped to have provided a design (or any guidance for a reviewer) for your implementation. (I took a long time to digest what was going on and I know a couple others had tried to look at the implementation and couldn't figure it out.)

[ChrisChiasson]

If you look two lines above the Eigen template specialization, it has special comment commands to turn doxygen off before it parses that particular template... because it will blow up the documentation build otherwise. It isn't like I was intentionally trying to make it obscure*.

I also laid out in detail, and others discussed above, why I closed the previous PR, which had nothing to do with a desire to avoid unit tests or documentation.

With that said, if what you want is the removal of the inheritance trick, more documentation, and more unit tests, then I think we can do a new PR as we talked about. It will take me a few weeks until my (offshore cranes) code at work is at a point where this stuff will be needed again (and therefore able to justify the bit of time it will take to get the complex implementation upstreamed).

*It could be argued that I could have put the doxygen documentation in there even though I was forced to turn doxygen parsing off. I can do that, though I will have to figure out what to write since it isn't really the start of a template, just a specialization of an existing one in a library.