Feature/issue 123 complex numbers with vars

Discourse use cases and spec discussion: http://discourse.mc-stan.org/t/complex-number-support-in-math-library-autodiff/3511
Submission Checklist

[ MinGW GCC 7.2 fails on one of the unit tests due to a compiler bug that I can't work around inside this set of patch commits, so no] Run unit tests: ./runTests.py test/unit
[ $ make cpplint
make: execvp: python: Argument list too long
make: *** [make/cpplint:5: cpplint] Error 127
] Run cpplint: make cpplint
[X] Declare copyright holder and open-source license: see below

Summary:
Initial attempt at adding complex number support to var, as referenced in issue #123
Intended Effect:
Complex numbers work
How to Verify:
Run unit test
Side Effects:
I am sure there are many that I haven't thought of. One I can think of is that the interface of the complex class in stan math has a more flexible interface than normal std::complex. This is because the binary operators use unconstrained templates. This could have "interesting" interactions.
Documentation:
I added some inline documentation using doxygen syntax.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Oil States
By submitting this pull request, the copyright holder is agreeing to 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/)

Here is one page with some information sort of like this: https://github.com/stan-dev/stan/wiki/Code-Quality
Anyone know if we have a single page we can link new devs to? @syclik maybe?

I see, what I was looking for (description of code layout and dependencies) is here:
https://github.com/stan-dev/stan/wiki/Contributing-New-Functions-to-Stan
We need to prioritize getting that single getting-started page for new contributors.

I swear I’ve written it out before, but I can’t find it.
I think we should have separate ones for Stan and Math.

On Mar 7, 2018, at 4:17 PM, Bob Carpenter<EMAIL_ADDRESS>wrote:
I see, what I was looking for (description of code layout and dependencies) is here:
https://github.com/stan-dev/stan/wiki/Contributing-New-Functions-to-Stan
We need to prioritize getting that single getting-started page for new contributors.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

The closest thing we have is the top-level wiki on stan-dev/stan, which has a lot of links:
https://github.com/stan-dev/stan/wiki

This is probably a dumb newbie question (apologies in advance): Which code is on the var path that includes fvar?

Don't worry! Things are a little complicated and we're still apparently working on our intro material, haha.
We've tried to keep var and fvar code completely separate so far. We have a mix directory that is for higher order code that would likely be using fvar<var> and fvar<fvar<var>>, but you only put code in there if you need to specialize for that case.
I think your code could go in prim, though we'd then take out the fvar and var forward declarations and the enable_if and lose a little bit of type safety there.
PS Do you have experience using forward declarations to reduce compile times? It's something I've been thinking about lately but haven't done before. Compile time is a big concern of ours.

The enable_if is there because ctor will fire on R = std::complex otherwise. I will try to reformulate the enable_if. Also, I took note about the comment in the c11 test file where we are allowed to use C++14 features, so that should help shorten the enable_if and remove any reference to var or fvar. I am still struggling with how to use github in this workflow, so apologies if I do it wrong...

You have a using fvar in the complex code, then var includes the complex code and hence fvar.

Yea, I had thought I was being clever by putting the file way up high, out of the rev and fwd directories, but I now see the policy isn't just a file/directory level-thing, but an include thing. Sorry about that.

Certainly agree. Btw, I only had them as fwd declarations (their
definitions werent imported), but I still concede it is better to avoid
having the symbol all together... and I have removed them from the last
commit.
On Wed, Mar 7, 2018, 6:51 PM Bob Carpenter<EMAIL_ADDRESS>wrote:

Right---the reason it's at the include level is to prevent code that only
needs reverse-mode (like everything in Stan at the moment) from having to
import and parse all the forward-mode code. It also avoids circular
includes, which makes the include logic easier to understand. It also makes
taming all the include-order dependencies for the metaprograms simpler.

On Mar 7, 2018, at 6:28 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
Yea, I had thought I was being clever by putting the file way up high,
out of the rev and fwd directories, but I now see the policy isn't just a
file/directory level-thing, but an include thing. Sorry about that.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371338948, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-3Ie99I0GqFIUDkn3bQBycwcORCRks5tcICXgaJpZM4ShF_6
.

Thansk for the clarification---you're absolutely right.  Just a simple forward declaration isn't going to cause a bunch of code to be brought in as I was suggesting---it's not an include.
We still don't want to bring in the symbol because nothing will be able to bring in the declaration or definition.

I'm not sure. (I've also been sick with a headache, so not completely on... I'll take a look again tomorrow.)
@bob-carpenter, thoughts? I'd trust his judgement if he gets to it before I do.

I just looked back at the code a little more carefully. Take all I've said with a grain of salt. I may not have gotten all the nuance there. (I'm still trying to get around the fact that stan::math::internal::complex<T> is a subclass of std::complex<T>, but then we template specialize std::complex<stan::math::var> as a subclass of stan::math::internal::complex.)
So... I'm not sure exactly where it belongs. If we never need to template specialize for double, I'm almost tempted to suggest just specializing std::complex<var> and std::complex<fvar<T>> in their respective folders and not have anything live in prim. That would duplicate code -- based on the way you've handled the operators -- but I think it may be clearer? I'm not sure yet.

Allow me to comment on that — one of the reasons that we haven’t
tackled complex numbers is exactly this reason.  In order to implement
autodiff with complex numbers correctly we’d have to specialize the
entire algebra of complex numbers to our autodiff types.  This kind
of cherry picking of some functionality will always leave holes that
compromise the UX.  Ultimately we really want to bite the bullet and
specialize all of the operations or back off and just implement the
real outputs of common complex operations (for example moduli
or the power/phase output of FFTs).

On Mar 7, 2018, at 9:31 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
The reason I avoided directly specializing std::complex on var or fbar is
that the full class is large, and the specialization would need to
reimplement all of it. :-(
On Wed, Mar 7, 2018, 8:26 PM Daniel Lee<EMAIL_ADDRESS>wrote:

I just looked back at the code a little more carefully. Take all I've said
with a grain of salt. I may not have gotten all the nuance there. (I'm
still trying to get around the fact that stan::math::internal::complex
is a subclass of std::complex, but then we template specialize
std::complexstan::math::var as a subclass of
stan::math::internal::complex.)
So... I'm not sure exactly where it belongs. If we never need to template
specialize for double, I'm almost tempted to suggest just specializing
std::complex and std::complex<fvar> in their respective folders
and not have anything live in prim. That would duplicate code -- based on
the way you've handled the operators -- but I think it may be clearer? I'm
not sure yet.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371356813, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-6ZIHP_5xapx8AyAhJHdgwyWrcOOks5tcJa6gaJpZM4ShF_6
.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/pull/789#issuecomment-371357779, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdNlpglYzvoXBZoBCW39uhgmzCmTRY3ks5tcJgFgaJpZM4ShF_6.

Which examples are relevant depends deeply on the specific
application being considered.  We haven’t had one application
be requested enough to warrant its inclusion, just a lot of
particular applications that are all related to complex numbers.
And that’s the challenge.  Implementing a complete complex
algebra would enable most of those applications, but no single
application has enough demand that a specialized implementation
would yield significant use.
Might be worth a Discourse thread requesting input from users.
My main point is that an incomplete implementation of a complex
type, without the full algebra, will likely cause problems as it will
be hard to communicate what is implemented and what is not.
Unless the partial implementation is wrapped in specific functions,
as opposed to enable_if’ing the hell out of the complex class, but
then that brings us back to the beginning.

On Mar 7, 2018, at 10:42 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
Just one or two short ones involving derivative propagation so I can wrap
my head around it tomorrow. In the back of my mind I am envisioning some
kind scheme to disable specific Stan functions that are only valid on
reals, or alternatively only enable functions valid on complexes with
everything else failing to compile by default.
On Wed, Mar 7, 2018, 9:10 PM Michael Betancourt<EMAIL_ADDRESS>wrote:

What do you mean by derivative examples? Specific operations
over complex numbers?

On Mar 7, 2018, at 9:57 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
Can you give 1 or 2 derivative examples you would like to cover so I can
start wrapping my mind around it?
On Wed, Mar 7, 2018, 8:54 PM Michael Betancourt <
<EMAIL_ADDRESS>wrote:

Allow me to comment on that — one of the reasons that we haven’t
tackled complex numbers is exactly this reason. In order to implement
autodiff with complex numbers correctly we’d have to specialize the
entire algebra of complex numbers to our autodiff types. This kind
of cherry picking of some functionality will always leave holes that
compromise the UX. Ultimately we really want to bite the bullet and
specialize all of the operations or back off and just implement the
real outputs of common complex operations (for example moduli
or the power/phase output of FFTs).

On Mar 7, 2018, at 9:31 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
The reason I avoided directly specializing std::complex on var or
fbar is
that the full class is large, and the specialization would need to
reimplement all of it. :-(
On Wed, Mar 7, 2018, 8:26 PM Daniel Lee<EMAIL_ADDRESS>wrote:

I just looked back at the code a little more carefully. Take all
I've
said
with a grain of salt. I may not have gotten all the nuance there.
(I'm
still trying to get around the fact that
stan::math::internal::complex
is a subclass of std::complex, but then we template specialize
std::complexstan::math::var as a subclass of
stan::math::internal::complex.)
So... I'm not sure exactly where it belongs. If we never need to
template
specialize for double, I'm almost tempted to suggest just
specializing
std::complex and std::complex<fvar> in their respective
folders
and not have anything live in prim. That would duplicate code --
based
on
the way you've handled the operators -- but I think it may be
clearer?
I'm
not sure yet.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/stan-dev/math/pull/789#issuecomment-371356813
,
or mute
the thread
<

https://github.com/notifications/unsubscribe-auth/ALTv-6ZIHP_5xapx8AyAhJHdgwyWrcOOks5tcJa6gaJpZM4ShF_6

.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <
https://github.com/stan-dev/math/pull/789#issuecomment-371357779>, or
mute the thread <

https://github.com/notifications/unsubscribe-auth/ABdNlpglYzvoXBZoBCW39uhgmzCmTRY3ks5tcJgFgaJpZM4ShF_6

.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371361637,
or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ALTv-0QS6JXvtDVGqwfDFkVRJjofO6X1ks5tcJ1VgaJpZM4ShF_6

.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <
https://github.com/stan-dev/math/pull/789#issuecomment-371362204>, or
mute the thread <
https://github.com/notifications/unsubscribe-auth/ABdNlnDqgZA-cSsxxNzpiL042RUv2NKpks5tcJ4ggaJpZM4ShF_6
.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371364147, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-zxsAn5uqD14YO7W2eCciHqfHo_sks5tcKEJgaJpZM4ShF_6
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/pull/789#issuecomment-371369131, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdNltZT2imDDu6dPc8QXBjPh42nnxBPks5tcKjDgaJpZM4ShF_6.

Since we need also need a spec before we can really evaluate a PR (as Bob mentioned), I went ahead and made a discourse thread asking for user input on applications. We can also use it for spec discussion (or we could make a new github issue for that, that could be better because we can summarize the comments at the top). Here's the link to the discourse thread: http://discourse.mc-stan.org/t/complex-number-support-in-math-library-autodiff/3511

Feel free to keep pushing. We may close the pull request if it seems like
it's far from being ready (closed doesn't mean won't accept; any open pull
request will test on every push and resources can get scarce at times).
What are ship RAOs?
On Thu, Mar 8, 2018 at 9:16 AM, ChrisChiasson<EMAIL_ADDRESS>wrote:

The travis build passed, but the jenkins status says it can't be built. I
noticed I don't seem to be able to load the details for jenkins, so I can't
tell what is wrong. I intend to keep working to add fvar support so I can
use complex numbers in an optimization problem involving ship RAOs. Let me
know if that isn't ok, because the pushes to my remote repo branch are
going to push notifications into this thread, though I will keep it to a
minimum.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371498857, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_F5SlpDTWlkUkSta2yKq-bYFZcspLks5tcT00gaJpZM4ShF_6
.

Just to reiterate - Bob wants a spec first, which likely requires some
description of the use case. Code without some kind of spec or goal is
difficult to evaluate.
On Thu, Mar 8, 2018 at 09:26 Daniel Lee<EMAIL_ADDRESS>wrote:

Feel free to keep pushing. We may close the pull request if it seems like
it's far from being ready (closed doesn't mean won't accept; any open pull
request will test on every push and resources can get scarce at times).
What are ship RAOs?
On Thu, Mar 8, 2018 at 9:16 AM, ChrisChiasson<EMAIL_ADDRESS>wrote:

The travis build passed, but the jenkins status says it can't be built. I
noticed I don't seem to be able to load the details for jenkins, so I
can't
tell what is wrong. I intend to keep working to add fvar support so I can
use complex numbers in an optimization problem involving ship RAOs. Let
me
know if that isn't ok, because the pushes to my remote repo branch are
going to push notifications into this thread, though I will keep it to a
minimum.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371498857, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAZ_F5SlpDTWlkUkSta2yKq-bYFZcspLks5tcT00gaJpZM4ShF_6
.

—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371501879, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxJ7KKcuORvX6QVDKRHljckhqrpui15ks5tcT-tgaJpZM4ShF_6
.

+1. Definitely design and spec first. Thanks for keeping us on track, Sean.
(When I said “pushing” I meant git push, not pushing forward without a design. Sorry for the confusion.)

On Mar 8, 2018, at 10:06 AM, seantalts<EMAIL_ADDRESS>wrote:
Just to reiterate - Bob wants a spec first, which likely requires some
description of the use case. Code without some kind of spec or goal is
difficult to evaluate.
On Thu, Mar 8, 2018 at 09:26 Daniel Lee<EMAIL_ADDRESS>wrote:

Feel free to keep pushing. We may close the pull request if it seems like
it's far from being ready (closed doesn't mean won't accept; any open pull
request will test on every push and resources can get scarce at times).
What are ship RAOs?
On Thu, Mar 8, 2018 at 9:16 AM, ChrisChiasson<EMAIL_ADDRESS>wrote:

The travis build passed, but the jenkins status says it can't be built. I
noticed I don't seem to be able to load the details for jenkins, so I
can't
tell what is wrong. I intend to keep working to add fvar support so I can
use complex numbers in an optimization problem involving ship RAOs. Let
me
know if that isn't ok, because the pushes to my remote repo branch are
going to push notifications into this thread, though I will keep it to a
minimum.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371498857, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAZ_F5SlpDTWlkUkSta2yKq-bYFZcspLks5tcT00gaJpZM4ShF_6
.

—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371501879, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxJ7KKcuORvX6QVDKRHljckhqrpui15ks5tcT-tgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

I'm still not sure what this PR does and would very much like to see a matching issue with a description of the functionality.  Daniel---can you write one out if you understand it?  I barely understand complex numbers and don't at all understand the intent of this PR.
On a technical note, subclasses will inherit implementations, whereas template specializations won't.  We have to be careful with what's virtual to make sure we don't overload non-virtual methods that would get sliced by references to the base class.

On Mar 7, 2018, at 9:31 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
The reason I avoided directly specializing std::complex on var or fbar is
that the full class is large, and the specialization would need to
reimplement all of it. :-(
On Wed, Mar 7, 2018, 8:26 PM Daniel Lee<EMAIL_ADDRESS>wrote:

I just looked back at the code a little more carefully. Take all I've said
with a grain of salt. I may not have gotten all the nuance there. (I'm
still trying to get around the fact that stan::math::internal::complex
is a subclass of std::complex, but then we template specialize
std::complexstan::math::var as a subclass of
stan::math::internal::complex.)
So... I'm not sure exactly where it belongs. If we never need to template
specialize for double, I'm almost tempted to suggest just specializing
std::complex and std::complex<fvar> in their respective folders
and not have anything live in prim. That would duplicate code -- based on
the way you've handled the operators -- but I think it may be clearer? I'm
not sure yet.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-371356813, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-6ZIHP_5xapx8AyAhJHdgwyWrcOOks5tcJa6gaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

BTW, I'm continuing to work on this for fvars, making changes accordingly: It may make sense to document what you want from complex numbers in the short term, either here or in discourse, so I can try to take that into account while I build what I need.

I think we're asking you to document what you're aiming to do before we merge your PR, haha.

Once I finish my optimization, I will try to figure out how this project writes specs and submit one.

We don't have ay specific format, just something that lets us know what operations you want to work, what's not supported, that kind of thing. If you're dead set on coding before getting feedback on a spec, your tests might function as a sort of very detailed and functional spec, which I think would be great.

Ok, there are some unit tests in there now checking multiplication, division, and abs operations, but I imagine the project will need better ones.

 It's beginning to look like the integrate_ode_bdf / rk45 functions aren't intended to be able to transmit fvar information from their inputs such as y0 or theta to their output. @wds15 ?

That's correct.
The ode solvers can't handle forward mode autodiff now.

 It's beginning to look like the integrate_ode_bdf / rk45 functions aren't intended to be able to transmit fvar information from their inputs such as y0 or theta to their output. @wds15 ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Earlier this week, I got my error minimization calculations working with gradient descent rather than full Newton's method due to the point mentioned above about the ode solvers. In a few days, I will add a small example showing how to use the code I've added (aside from the unit tests already in theis PR). After that, I will attempt to make the PR address the wider concerns everyone has voiced.

Trying to fix the CI errors. Went to https://travis-ci.org/stan-dev/math/jobs/381043994/config and saw that clang-3.8 and libc++-dev are being used. On new Ubuntu LTS 16.04 instances on Google Compute Engine, it only has clang-4.0. I did install libc++-dev. I am deliberately not installing libstdc++ because of comments in other threads that libc++ needs to be supported.
I have tried specifying CXX="clang++-4.0 -stdlib=libc++" on the runTests.py command line, but it causes some kind of parsing error, putting make in verbose mode. I also tried it with single quotes. I also tried it with separating out CXX=clang++-4.0 CXXFLAGS=-stdlib=libc++, but that overerides all the other CXXFLAGS. Same if I try it with +=.
I have also tried putting
CXX=clang++-4.0
CXXFLAGS+=-stdlib=libc++
in the file local under the make folder, as well as the same contents in the ~/.config/stan/make.local file recommended at https://github.com/stan-dev/math/wiki/Developer-Doc
./runTests.py CXX=clang++-4.0 test/unit/math/rev/scal/fun/abs_test
------------------------------------------------------------
make -j1 CXX=clang++-4.0 test/unit/math/rev/scal/fun/abs_test
In file included from test/unit/math/rev/scal/fun/abs_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/core.hpp:4:
In file included from ./stan/math/rev/core/autodiffstackstorage.hpp:4:
./stan/math/memory/stack_alloc.hpp:8:10: fatal error: 'cstdlib' file not found
#include <cstdlib>
         ^~~~~~~~~
1 error generated.
clang++-4.0 -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/cvodes-3.1.0/include -isystem lib/idas-2.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 -stdlib=libc++ -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe  -c -o lib/gtest_1.7.0/src/gtest-all.o lib/gtest_1.7.0/src/gtest-all.cc
In file included from lib/gtest_1.7.0/src/gtest-all.cc:39:
lib/gtest_1.7.0/include/gtest/gtest.h:54:10: fatal error: 'limits' file not found
#include <limits>
         ^~~~~~~~
1 error generated.
<builtin>: recipe for target 'lib/gtest_1.7.0/src/gtest-all.o' failed
make: *** [lib/gtest_1.7.0/src/gtest-all.o] Error 1
make -j1 CXX=clang++-4.0 test/unit/math/rev/scal/fun/abs_test failed
exit now (05/20/18 01:27:00 UTC)

For Jenkins, you are getting cpplint errors - check out the make target for cpplint and see if you can run a similar command on your modified files, or just use the Jenkins output at the link (it should tell you which files are problematic and how to fix the issues).
For Travis, I'm not sure where you're seeing what you're seeing, but the errors I see immediately at the Travis job link are that you're using C++17 stuff?
./stan/math/cplx/complex.hpp:29:1: warning: inline variables are a C++17 extension [-Wc++17-extensions]
inline constexpr bool is_cplx_v = is_cplx<T>::value;

I did notice those things and do intend to take care of those as well. I
just want to have it so I can run the tests without consuming your CI
resources (I had tried on Linux before and failed). Did my questions make
sense? Do you guys do the clang tests without the GCC STD libs?
On Sun, May 20, 2018, 11:42 AM seantalts<EMAIL_ADDRESS>wrote:

For Jenkins, you are getting cpplint errors - check out the make target
for cpplint and see if you can run a similar command on your modified
files, or just use the Jenkins output at the link (it should tell you which
files are problematic and how to fix the issues).
For Travis, I'm not sure where you're seeing what you're seeing, but the
errors I see immediately at the Travis job link are that you're using C++17
stuff?
./stan/math/cplx/complex.hpp:29:1: warning: inline variables are a C++17 extension [-Wc++17-extensions]
inline constexpr bool is_cplx_v = is_cplx::value;
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-390494989, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-wKz0q1_YQGQ3f5PtlyAx-hxngc9ks5t0Zz2gaJpZM4ShF_6
.

I tried to address the inline constexpr and cpplint issues. Still no luck with the tests. I guess we'll see what the integration servers say.

I misunderstood your question! Sorry about that.
I have had bunches of trouble with Ubuntu's C compiler packages when trying to use C++11. I have no idea why - seems fairly basic, but I am no Ubuntu wizard.
If you're trying to duplicate the travis environment, (which is Ubuntu 14.04, you can start with 14.04 and see how the .travis.yml file specified additional PPLs and packages to install: https://github.com/stan-dev/math/blob/develop/.travis.yml
So I think if you want clang, you want these additional PPLs: https://github.com/stan-dev/math/blob/develop/.travis.yml#L22
And then to apt-get install these packages: https://github.com/stan-dev/math/blob/develop/.travis.yml#L23

Also just want to confirm that you should indeed be using make/local for these flags (though in our travis config we just set environment variables). You can see how Jenkins does this in the Jenkinsfile

From the latest error messages, it appears that with older compilers, eigen changes the 3rd template parameter of ScalarBinaryOpTraits. In modern compilers, the 3rd parameter is a template that repeats the first two parameter types. That enabled me to stuff the 2nd template parameter full of std::enable_if, and then pick up the real T2 from the nested type in the 3rd paramter. Now I can't do that, so I am kind of at a loss. I will think about it.

Oye, that sounds frustrating. I'm not sure how much we care about enable_ifs when used to be explicit about what a template works with - we often rely on people to correctly use these template programs rather than having the compiler check that templates are only used under certain conditions. Not sure if that's what you were using it for, though.

To be honest, I am not actually sure now that I understood the error messages the compiler spit out. I have a feeling it is going to take several iterations to get something working. Unfortunately that template is used to let Eigen know when an operation is supported, so it has to be precise, and it can't enable itself in any situation in which the existing Eigen templates enable themselves or there will be ambiguity compiler error messages.

@yashikno whatever you just added in commit b158578 just blew up my configuration.  Now the plain var_test.cpp is saying it can't find cvodes.  Please fix. Here is your commit diff, which is very large: https://github.com/stan-dev/math/commit/655bba3cc5609ce21fed0309cdada6dc46966a2c
chris_chiasson@stan-test-14-04:~/stan-dev/math$ clang++-3.8 -DSTAN_THREADS -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/cvodes-3.1.0/include -isystem lib/idas-2.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 -stdlib=libc++ -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/core/var_test.o test/unit/math/rev/core/var_test.cpp
In file included from test/unit/math/rev/core/var_test.cpp:1:
In file included from ./stan/math.hpp:4:
In file included from ./stan/math/rev/mat.hpp:59:
./stan/math/rev/mat/functor/cvodes_utils.hpp:4:10: fatal error: 'cvodes/cvodes.h' file not found

That sounds frustrating. I'm hoping we see the day soon where we pick a single recent compiler to support and install!

@seantalts Even if it were recent GCC and clang, with both libstdc++ and libc++, that would still help a lot.

In order to finish this, I need some help interpreting what is going on with clang. I haven't been able to positively identify what piece of libc++'s complex implementation is responsible for leaving generic complex types in __1 namespace limbo, while seemingly also declaring the float, double, and long double versions in the std namespace. I can see that certain macros are manipulating "visibility", which is supposed to be related to vtables and such. Even if I could find exactly why generic complex types are in __1 and the double, long double, and floats don't seem to be (of course, one never knows because there is not an easy way to force clang to print fully qualified types for everything), I am not having luck forcing my functions to show up in the list of considered functions for ADL for generic types. I realize that because __1 is an inline namespace, that it should not affect ADL, but it appears to anyway. Possibly useful links:
https://github.com/llvm-mirror/libcxx/blob/master/include/complex
http://bcain-llvm.readthedocs.io/projects/libcxx/en/latest/DesignDocs/VisibilityMacros.html
FYI the four travis tests that failed ( https://travis-ci.org/stan-dev/math/builds/384295445?utm_source=github_status&utm_medium=notification ) aren't showing the problem I'm talking about, and I can't see the jenkins output due to my employer's corporate firewall ( http://d1m1s1b1.stat.columbia.edu:8080/job/Math Pipeline/job/PR-789/37/display/redirect ). The immediate problem arose while trying to alter equality tests to subtract the left and the right hand sides and compare the absolute residual to a tolerance. Yes, I am aware the tests show other problems, but one thing at a time...
Two commands to try:
clang++-3.8 -DSTAN_THREADS -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 -stdlib=libc++ -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/core/var_test.o test/unit/math/rev/core/var_test.cpp

clang++-3.8 -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 -stdlib=libc++ -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe  -c -o test/unit/math/rev/core/var_test.o test/unit/math/mix/mat/functor/autodiff_test.cpp

We're down to two errors (from four) in Travis and one (?) error in Jenkins. In Travis, the error in var_test.cpp is due to logb and another function called in clang's complex division routine. I need to mask those (as mentioned in previous posts). In Jenkins clang's dims_test.cpp there is a complaint about an ambiguous overload for vector<fvar<fvar>>.push_back(-31.2), where it can't decide between the const reference overload and the rvalue reference overload. Apparently the vector<fvar>.push_back(-31.2) call higher in the same file was ok though.

I am going to clean some things up and then I'm checking out. The relevant tests work on clang 5.0 with libc++ or libstdc++, or on the latest gcc with libstdc++. Clang 3.8 on libc++ is refusing to accept the best method(s) I can come up with to overrideits operator/ for complex. Currently, that involves creating a base function template for std::complex<fvar> that overloads with clang's complex operator/ template. The overload works perfectly on clang 5, but doesn't work on 3.8.

Have not looked at this closely yet, but in one test, it does not crash on some Eigen stuff that uses complex arithmetic internally that used to crash when given a matrix of vars. So, that alone would be a reasonably big win.

I, for one, like tests such as the following, which generates a random asymmetric matrix and calculates the autodiff Jacobian when you do some Eigen stuff that entails complex arithmetic inside the struct to obtain an identity matrix. Then check that it is pretty close to an identity matrix and that the autodiffed Jacobian is pretty close to a zero matrix. This one actually passes
#include <stan/math/rev/mat.hpp>
#include <gtest/gtest.h>
#include <vector>

struct make_I {
  template<typename T>
  Eigen::Matrix<T, Eigen::Dynamic, 1>
  operator()(const Eigen::Matrix<T, Eigen::Dynamic, 1>& a) const {
    typedef Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> matrix_t;
    const int K = static_cast<int>(sqrt(a.rows()));
    matrix_t A(K, K);
    int pos = 0;
    for (int i = 0; i < K; i++) for (int j = 0; j < K; j++) A(i,j) = a[pos++];
    Eigen::EigenSolver<matrix_t> es(A);
    matrix_t I = (es.eigenvectors().inverse() * A * es.eigenvectors() * es.eigenvalues().asDiagonal().inverse()).real();
    I.resize(K * K, 1);
    return I;
  }
};

TEST(MathMatrix, eigen_decomposition) {
  using stan::math::matrix_d;
  int K = 9;
  matrix_d A(K, K);
  A.setRandom();
  matrix_d J;
  Eigen::VectorXd f_x;
  A.resize(K * K, 1);
  make_I f;
  stan::math::jacobian(f, A, f_x, J);
  int pos = 0;
  for (int j = 0; j < K; j++) {
    for (int i = 0; i < j; i++)
      EXPECT_NEAR(f_x[pos++], 0, 1e-12);
    EXPECT_NEAR(f_x[pos++], 1, 1e-12);
    for (int i = j + 1; i < K; i++)
      EXPECT_NEAR(f_x[pos++], 0, 1e-12);
  }
  for (int i = 0; i < J.rows(); i++) for (int j = 0; j < J.cols(); j++)
    EXPECT_NEAR(J(i,j), 0, 1e-12);
}

Thanks for the example, @bgoodri. Agreed that it's good to have that type
of test.
But... it also helps to have deterministic tests that don't relax the
accuracy of the tests. We've actually run into issues with gradients being
off by under 1e-12 that we let slide using EXPECT_NEAR that tanked
performance. We don't want to relax the accuracy across the whole range of
valid inputs. Hope that makes sense.
On Wed, Jun 20, 2018 at 7:23 PM<EMAIL_ADDRESS>
I, for one, like tests such as the following, which generates a random
asymmetric matrix and calculates the autodiff Jacobian when you do some
Eigen stuff that entails complex arithmetic inside the struct to obtain an
identity matrix. Then check that it is pretty close to an identity matrix
and that the autodiffed Jacobian is pretty close to a zero matrix. This one
actually passes
#include <stan/math/rev/mat.hpp>
#include <gtest/gtest.h>
#include
struct make_I {
template
Eigen::Matrix<T, Eigen::Dynamic, 1>
operator()(const Eigen::Matrix<T, Eigen::Dynamic, 1>& a) const {
typedef Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> matrix_t;
const int K = static_cast(sqrt(a.rows()));
matrix_t A(K, K);
int pos = 0;
for (int i = 0; i < K; i++) for (int j = 0; j < K; j++) A(i,j) = a[pos++];
Eigen::EigenSolver<matrix_t> es(A);
matrix_t I = (es.eigenvectors().inverse() * A * es.eigenvectors() * es.eigenvalues().asDiagonal().inverse()).real();
I.resize(K * K, 1);
return I;
}
};
TEST(MathMatrix, eigen_decomposition) {
using stan::math::matrix_d;
int K = 9;
matrix_d A(K, K);
A.setRandom();
matrix_d J;
Eigen::VectorXd f_x;
A.resize(K * K, 1);
make_I f;
stan::math::jacobian(f, A, f_x, J);
int pos = 0;
for (int j = 0; j < K; j++) {
for (int i = 0; i < j; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
EXPECT_NEAR(f_x[pos++], 1, 1e-12);
for (int i = j + 1; i < K; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
}
for (int i = 0; i < J.rows(); i++) for (int j = 0; j < J.cols(); j++)
EXPECT_NEAR(J(i,j), 0, 1e-12);
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398928093, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_FwZhaCg2McVsRoeX40vfxRZfw6BTks5t-tmCgaJpZM4ShF_6
.

I just pulled 1e-12 out of thin air. What should it be? I wouldn't be
surprised if it could achieve better than that, but complex eigenvalues
won't be accurate to machine precision.
On Wed, Jun 20, 2018 at 8:33 PM Daniel Lee<EMAIL_ADDRESS>wrote:

Thanks for the example, @bgoodri. Agreed that it's good to have that type
of test.
But... it also helps to have deterministic tests that don't relax the
accuracy of the tests. We've actually run into issues with gradients being
off by under 1e-12 that we let slide using EXPECT_NEAR that tanked
performance. We don't want to relax the accuracy across the whole range of
valid inputs. Hope that makes sense.
On Wed, Jun 20, 2018 at 7:23 PM<EMAIL_ADDRESS>
I, for one, like tests such as the following, which generates a random
asymmetric matrix and calculates the autodiff Jacobian when you do some
Eigen stuff that entails complex arithmetic inside the struct to obtain
an
identity matrix. Then check that it is pretty close to an identity matrix
and that the autodiffed Jacobian is pretty close to a zero matrix. This
one
actually passes
#include <stan/math/rev/mat.hpp>
#include <gtest/gtest.h>
#include
struct make_I {
template
Eigen::Matrix<T, Eigen::Dynamic, 1>
operator()(const Eigen::Matrix<T, Eigen::Dynamic, 1>& a) const {
typedef Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> matrix_t;
const int K = static_cast(sqrt(a.rows()));
matrix_t A(K, K);
int pos = 0;
for (int i = 0; i < K; i++) for (int j = 0; j < K; j++) A(i,j) =
a[pos++];
Eigen::EigenSolver<matrix_t> es(A);
matrix_t I = (es.eigenvectors().inverse() * A * es.eigenvectors() *
es.eigenvalues().asDiagonal().inverse()).real();
I.resize(K * K, 1);
return I;
}
};
TEST(MathMatrix, eigen_decomposition) {
using stan::math::matrix_d;
int K = 9;
matrix_d A(K, K);
A.setRandom();
matrix_d J;
Eigen::VectorXd f_x;
A.resize(K * K, 1);
make_I f;
stan::math::jacobian(f, A, f_x, J);
int pos = 0;
for (int j = 0; j < K; j++) {
for (int i = 0; i < j; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
EXPECT_NEAR(f_x[pos++], 1, 1e-12);
for (int i = j + 1; i < K; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
}
for (int i = 0; i < J.rows(); i++) for (int j = 0; j < J.cols(); j++)
EXPECT_NEAR(J(i,j), 0, 1e-12);
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398928093, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAZ_FwZhaCg2McVsRoeX40vfxRZfw6BTks5t-tmCgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398939490, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrquiP43Ta9Oh8gRrTT_KeZ8qeue1Tks5t-unHgaJpZM4ShF_6
.

And that’s the issue with random testing. Unless the function is
numerically well-behaved in the whole domain, we have to relax the
tolerance. That’s great because something like this will catch gross errors
that we would miss! But, it misses finer numerical implementation errors
that should also be checked.
All I’m trying to say is that we could use both. I’d err on the side of
only having deterministic ones (if I had to choose one or the other)
because it won’t fail dependent on run, it shows the intent of the
developer (the examples are within the typical range of what the developer
expects), and it’s simpler to write and debug test code. But yes, we could
use both
On Wed, Jun 20 2018 at 11:14 PM<EMAIL_ADDRESS>wrote:

I just pulled 1e-12 out of thin air. What should it be? I wouldn't be
surprised if it could achieve better than that, but complex eigenvalues
won't be accurate to machine precision.
On Wed, Jun 20, 2018 at 8:33 PM Daniel Lee<EMAIL_ADDRESS>wrote:

Thanks for the example, @bgoodri. Agreed that it's good to have that type
of test.
But... it also helps to have deterministic tests that don't relax the
accuracy of the tests. We've actually run into issues with gradients
being
off by under 1e-12 that we let slide using EXPECT_NEAR that tanked
performance. We don't want to relax the accuracy across the whole range
of
valid inputs. Hope that makes sense.
On Wed, Jun 20, 2018 at 7:23 PM<EMAIL_ADDRESS>
I, for one, like tests such as the following, which generates a random
asymmetric matrix and calculates the autodiff Jacobian when you do some
Eigen stuff that entails complex arithmetic inside the struct to obtain
an
identity matrix. Then check that it is pretty close to an identity
matrix
and that the autodiffed Jacobian is pretty close to a zero matrix. This
one
actually passes
#include <stan/math/rev/mat.hpp>
#include <gtest/gtest.h>
#include
struct make_I {
template
Eigen::Matrix<T, Eigen::Dynamic, 1>
operator()(const Eigen::Matrix<T, Eigen::Dynamic, 1>& a) const {
typedef Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> matrix_t;
const int K = static_cast(sqrt(a.rows()));
matrix_t A(K, K);
int pos = 0;
for (int i = 0; i < K; i++) for (int j = 0; j < K; j++) A(i,j) =
a[pos++];
Eigen::EigenSolver<matrix_t> es(A);
matrix_t I = (es.eigenvectors().inverse() * A * es.eigenvectors() *
es.eigenvalues().asDiagonal().inverse()).real();
I.resize(K * K, 1);
return I;
}
};
TEST(MathMatrix, eigen_decomposition) {
using stan::math::matrix_d;
int K = 9;
matrix_d A(K, K);
A.setRandom();
matrix_d J;
Eigen::VectorXd f_x;
A.resize(K * K, 1);
make_I f;
stan::math::jacobian(f, A, f_x, J);
int pos = 0;
for (int j = 0; j < K; j++) {
for (int i = 0; i < j; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
EXPECT_NEAR(f_x[pos++], 1, 1e-12);
for (int i = j + 1; i < K; i++)
EXPECT_NEAR(f_x[pos++], 0, 1e-12);
}
for (int i = 0; i < J.rows(); i++) for (int j = 0; j < J.cols(); j++)
EXPECT_NEAR(J(i,j), 0, 1e-12);
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398928093, or
mute
the thread
<

https://github.com/notifications/unsubscribe-auth/AAZ_FwZhaCg2McVsRoeX40vfxRZfw6BTks5t-tmCgaJpZM4ShF_6

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398939490, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ADOrquiP43Ta9Oh8gRrTT_KeZ8qeue1Tks5t-unHgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-398963011, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_F7Ts2dJOoUq7X2mqLFH2y07tKZyqks5t-w-QgaJpZM4ShF_6
.

In order for there to both be a finer numerical implementation error and for K^2 + K^4 equations to hold simultaneously, the numerical implementation errors would have to cancel each other out in a particular way rather than accumulating. I do not see how that could be the case with eigenvalue or O(K^3) matrix decompositions generally that are a function of every var in the original matrix.

I buy your argument, but there’s a practical issue where the thing that’s
recommended is to relax the bounds instead of hunt down errors with these
sorts of tests. And different compilers have different numerical behavior.
If we have clear(er) guidelines, then we're good.
On Thu, Jun 21 2018 at 3:56 PM<EMAIL_ADDRESS>wrote:

In order for there to both be a finer numerical implementation error and
for K^2 + K^4 equations to hold simultaneously, the numerical
implementation errors would have to cancel each other out in a particular
way rather than accumulating. I do not see how that could be the case with
eigenvalue or O(K^3) matrix decompositions generally that are a function of
every var in the original matrix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-399223619, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_FyJ2Um3cDN9Hc0uWOIMRI7FC-0bbks5t-_pvgaJpZM4ShF_6
.

I pushed a bunch of tests, but I am not sure where they went to. Maybe it appears as a PR on @ChrisChiasson 's repo? Anyway, several of them fail with an error in copysign, such as
./runTests.py test/unit/math/rev/scal/fun/asinh_test.cpp
...
/usr/include/c++/v1/complex:1140:45: error: no matching function for call to 'copysign'
            return complex<_Tp>(__x.real(), copysign(__pi * _Tp(0.25), __x.imag()));
                                            ^~~~~~~~
test/unit/math/rev/scal/fun/asinh_test.cpp:70:12: note: in instantiation of function template specialization 'std::__1::asinh<stan::math::var>' requested here
  auto f = asinh(z);
...
/usr/include/c++/v1/math.h:1095:1: note: candidate template ignored: substitution failure [with _A1 = stan::math::var, _A2 = stan::math::zeroing<stan::math::var>]: no type named 'type' in
      'std::__1::__lazy_enable_if<false, std::__1::__promote<stan::math::var, stan::math::zeroing<stan::math::var>, void> >'
copysign(_A1 __lcpp_x, _A2 __lcpp_y) _NOEXCEPT
...

Yes and I'm happy with that. I think @bgoodri is advocating adding random
testing (for good reasons!). Having fixed tests falls short on that goal, I
think.
On Fri, Jun 22 2018 at 9:26 AM<EMAIL_ADDRESS>wrote:

The randoms could be pulled out of one of the runs and then stored in the
test. Then the tolerance could be dialed down as tight as it will pass.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-399441647, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_F-uxR0hdo9qwdJYKZmemaYKum9Ylks5t_PB3gaJpZM4ShF_6
.

This PR is unusual in that the primary question is that whether there are any critical functions that have not been implemented? A more typical PR would narrowly test whether individual functions are working as expected. I did several of those for the functions defined in the C++ standard, although I don't know where they ended up being pushed to.
Having a test that touches as many functions as possible is good for seeing if something is missing, although that can be accomplished with any fixed input. Attached is a coverage report for the eigenvalues test, but it only touches about 50 functions
complex_eigenvalues_edited.txt
.
Random input is good for testing numerical accuracy over the typical input space, but that can really only be judged over the long run after it gets merged and the unit tests get executed over and over again on different PRs.

Good news everyone! I found where my commits were pushed to https://github.com/stan-dev/math/commits/ChrisChiasson-feature/issue-123-complex-numbers-with-vars . Can we merge that into @ChrisChiasson 's branch and then try to fix the copysign error?

Without knowing which file triggered the compile error, it is hard to tell. It was impossible to #include the complex implementation from the rev/core/var.hpp file without either inducing an include loop or violating the organization of stan's directory structure ({prim,rev,fwd} , {scal, arr, mat}, etc...). The copysign function does have a rudimentary definition in this patch. I know stan tests in general try to be careful about their includes. Maybe try including rev/cplx.hpp in the test, or if that is too much, take a look at how some of the other functions do their includes. By the way, if you want write access to my repo fork, I will add you (didn't know other people couldn't access it... since the bots have been adding things the whole time)

BTW, currently only travis is green ( https://travis-ci.org/stan-dev/math/builds/394728837?utm_source=github_status&utm_medium=notification )... If anyone happens to know offhand the solution to the current doxygen error on Jenkins (not the Eigen one I was fighting the other day), please drop a line. Otherwise I'll take care of it later.

If you can get my commits and run ./runTests.py test/unit/math/rev/scal/fun/asinh_test.cpp, it will throw the compile error or really just call asinh(z) where z is a std::complex<stan::math::var>.

I haven't pulled in your commits, but I did send you a write access invitation to my repo about an hour or so ago. I read the test file you mentioned. It includes rev/scal.hpp, which in turn includes prim/scal.hpp, which in turn includes prim/scal/fun/copysign.hpp, so the definition should be available. Because both var and zeroing<var> are in stan::math, complex asinh's unqualified lookup should invoke ADL to find template<class T> auto copysign(T const&,T const&) in stan::math.  In that case, one of the compiler notes for the error message in clang should state why the template didn't take. It is possible that one of the arguments could have been var, while the other was zeroing<var>, which would not have allowed the template to instantiate (and then give zeroing<var> a chance to convert itself to var). In that case, a possible solution would be to change the copysign template to take two parameters template <class T, class U = T> auto copysign(T const&, U const&)

What is zeroing<var>?

It was impossible to #include the complex implementation from the rev/core/var.hpp file without either inducing an include loop or violating the organization of stan's directory structure ({prim,rev,fwd} , {scal, arr, mat}, etc...).

What specifically are you trying to include where?  Why would you need to add more complex implementation to prim?
The way we usually set things up, base templates go in prim and then specializations go in rev and fwd if available.  There isn't anything that should be impossible to include this way.

@bob-carpenter You are correct that it is not impossible to make everything work correctly and conform to the stan directory structure (as evidenced by the green Travis build). I was attempting to anticipate a hypothetical situation about a test file from bgoodri that I hadn't seen, where the test may have been including something like rev/core/var.hpp and maybe only one or two other files. In that case, the only way to get the test file to work without #include changes would have been to put part of the complex implementation in the rev/core/var.hpp file. I was relating what happens when using that approach and separating everything else out. zeroing<T> is something I decided to use to move as much of the complex<T> functionality as possible into prim.

I opened up the copysign template, so the new tests should work

That seems to bypass the copysign errors. But even after I cherry-pick your last commit, there are other compiler errors with the tests I added that are now on an alternative universe branch. One of which comes from running ./runTests.py test/unit/math/rev/scal/fun/acos_test.cpp:
make -j1 test/unit/math/rev/scal/fun/acos_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 -stdlib=libc++ -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -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/acos_test.o test/unit/math/rev/scal/fun/acos_test.cpp
In file included from test/unit/math/rev/scal/fun/acos_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/cplx.hpp:4:
In file included from ./stan/math/rev/cplx/var.hpp:4:
In file included from ./stan/math/rev/scal/meta/ad_promotable.hpp:4:
In file included from ./stan/math/rev/core.hpp:12:
In file included from ./stan/math/rev/core/gevv_vvv_vari.hpp:5:
In file included from ./stan/math/rev/core/var.hpp:7:
In file included from ./stan/math/prim/cplx/complex.hpp:4:
In file included from ./stan/math/prim/cplx/zeroing.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_arith_like.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_fr_var.hpp:6:
In file included from ./stan/math/prim/scal/meta/rm_complex.hpp:5:
/usr/include/c++/v1/complex:1391:39: error: no matching function for call to 'signbit'
            return complex<_Tp>(__pi, signbit(__x.imag()) ? -__x.real() : __x.real());
                                      ^~~~~~~
test/unit/math/rev/scal/fun/acos_test.cpp:83:12: note: in instantiation of function template specialization 'std::__1::acos<stan::math::var>' requested here
  auto f = acos(z);
           ^
/usr/include/c++/v1/cmath:328:25: note: candidate template ignored: disabled by 'enable_if' [with _A1 = stan::math::zeroing<stan::math::var>]
typename std::enable_if<std::is_arithmetic<_A1>::value, bool>::type
                        ^
In file included from test/unit/math/rev/scal/fun/acos_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/cplx.hpp:4:
In file included from ./stan/math/rev/cplx/var.hpp:4:
In file included from ./stan/math/rev/scal/meta/ad_promotable.hpp:4:
In file included from ./stan/math/rev/core.hpp:12:
In file included from ./stan/math/rev/core/gevv_vvv_vari.hpp:5:
In file included from ./stan/math/rev/core/var.hpp:7:
In file included from ./stan/math/prim/cplx/complex.hpp:4:
In file included from ./stan/math/prim/cplx/zeroing.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_arith_like.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_fr_var.hpp:6:
In file included from ./stan/math/prim/scal/meta/rm_complex.hpp:5:
/usr/include/c++/v1/complex:1392:37: error: no matching function for call to 'signbit'
        return complex<_Tp>(_Tp(0), signbit(__x.imag()) ? __x.real() : -__x.real());
                                    ^~~~~~~
/usr/include/c++/v1/cmath:328:25: note: candidate template ignored: disabled by 'enable_if' [with _A1 = stan::math::zeroing<stan::math::var>]
typename std::enable_if<std::is_arithmetic<_A1>::value, bool>::type
                        ^
In file included from test/unit/math/rev/scal/fun/acos_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/cplx.hpp:4:
In file included from ./stan/math/rev/cplx/var.hpp:4:
In file included from ./stan/math/rev/scal/meta/ad_promotable.hpp:4:
In file included from ./stan/math/rev/core.hpp:12:
In file included from ./stan/math/rev/core/gevv_vvv_vari.hpp:5:
In file included from ./stan/math/rev/core/var.hpp:7:
In file included from ./stan/math/prim/cplx/complex.hpp:4:
In file included from ./stan/math/prim/cplx/zeroing.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_arith_like.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_fr_var.hpp:6:
In file included from ./stan/math/prim/scal/meta/rm_complex.hpp:5:
/usr/include/c++/v1/complex:1404:39: error: no matching function for call to 'pow'
    complex<_Tp> __z = log(__x + sqrt(pow(__x, _Tp(2)) - _Tp(1)));
                                      ^~~
./stan/math/rev/scal/fun/pow.hpp:104:12: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'const stan::math::var' for 1st argument
inline var pow(const var& base, const var& exponent) {
           ^
./stan/math/rev/scal/fun/pow.hpp:148:12: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'double' for 1st argument
inline var pow(double base, const var& exponent) {
           ^
/usr/include/x86_64-linux-gnu/bits/mathcalls.h:153:17: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'double' for 1st argument
__MATHCALL_VEC (pow,, (_Mdouble_ __x, _Mdouble_ __y));
                ^
/usr/include/math.h:59:15: note: expanded from macro '__MATHCALL_VEC'
  __MATHCALL (function, suffix, args)
              ^
/usr/include/math.h:66:25: note: expanded from macro '__MATHCALL'
  __MATHDECL (_Mdouble_,function,suffix, args)
                        ^
/usr/include/math.h:68:22: note: expanded from macro '__MATHDECL'
  __MATHDECL_1(type, function,suffix, args); \
                     ^
/usr/include/math.h:76:31: note: expanded from macro '__MATHDECL_1'
  extern type __MATH_PRECNAME(function,suffix) args __THROW
                              ^
/usr/include/math.h:79:42: note: expanded from macro '__MATH_PRECNAME'
#define __MATH_PRECNAME(name,r) __CONCAT(name,r)
                                         ^
/usr/include/x86_64-linux-gnu/sys/cdefs.h:88:23: note: expanded from macro '__CONCAT'
#define __CONCAT(x,y)   x ## y
                        ^
/usr/include/c++/v1/cmath:949:46: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'float' for 1st argument
inline _LIBCPP_INLINE_VISIBILITY float       pow(float __x, float __y) _NOEXCEPT             {return powf(__x, __y);}
                                             ^
/usr/include/c++/v1/cmath:950:46: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'long double' for 1st argument
inline _LIBCPP_INLINE_VISIBILITY long double pow(long double __x, long double __y) _NOEXCEPT {return powl(__x, __y);}
                                             ^
./stan/math/rev/scal/fun/pow.hpp:120:12: note: candidate function not viable: no known conversion from 'const complex<stan::math::var>' to 'const stan::math::var' for 1st argument
inline var pow(const var& base, double exponent) {
           ^
/usr/include/c++/v1/complex:1171:1: note: candidate template ignored: could not match 'complex<type-parameter-0-0>' against 'stan::math::var'
pow(const complex<_Tp>& __x, const complex<_Tp>& __y)
^
/usr/include/c++/v1/complex:1179:1: note: candidate template ignored: could not match 'complex<type-parameter-0-1>' against 'stan::math::var'
pow(const complex<_Tp>& __x, const complex<_Up>& __y)
^
/usr/include/c++/v1/complex:1205:1: note: candidate template ignored: could not match 'complex<type-parameter-0-1>' against 'stan::math::var'
pow(const _Tp& __x, const complex<_Up>& __y)
^
/usr/include/c++/v1/cmath:961:1: note: candidate template ignored: substitution failure [with _A1 = std::__1::complex<stan::math::var>, _A2 = stan::math::var]: no type named 'type' in
      'std::__1::__promote<std::__1::complex<stan::math::var>, stan::math::var, void, false>'
pow(_A1 __x, _A2 __y) _NOEXCEPT
^
/usr/include/c++/v1/complex:1192:1: note: candidate template ignored: substitution failure [with _Tp = stan::math::var, _Up = stan::math::var]: no type named 'type' in
      'std::__1::__promote<stan::math::var, stan::math::var, void, false>'
pow(const complex<_Tp>& __x, const _Up& __y)
^
/usr/include/c++/v1/valarray:4634:5: note: candidate template ignored: disabled by 'enable_if' [with _Expr1 = std::__1::complex<stan::math::var>, _Expr2 = stan::math::var]
    __is_val_expr<_Expr1>::value && __is_val_expr<_Expr2>::value,
    ^
/usr/include/c++/v1/valarray:4648:5: note: candidate template ignored: disabled by 'enable_if' [with _Expr = std::__1::complex<stan::math::var>]
    __is_val_expr<_Expr>::value,
    ^
/usr/include/c++/v1/valarray:4668:1: note: candidate template ignored: substitution failure [with _Expr = stan::math::var]: no type named 'value_type' in 'stan::math::var'
pow(const typename _Expr::value_type& __x, const _Expr& __y)
^
In file included from test/unit/math/rev/scal/fun/acos_test.cpp:1:
In file included from ./stan/math/rev/scal.hpp:4:
In file included from ./stan/math/rev/cplx.hpp:4:
In file included from ./stan/math/rev/cplx/var.hpp:4:
In file included from ./stan/math/rev/scal/meta/ad_promotable.hpp:4:
In file included from ./stan/math/rev/core.hpp:12:
In file included from ./stan/math/rev/core/gevv_vvv_vari.hpp:5:
In file included from ./stan/math/rev/core/var.hpp:7:
In file included from ./stan/math/prim/cplx/complex.hpp:4:
In file included from ./stan/math/prim/cplx/zeroing.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_arith_like.hpp:4:
In file included from ./stan/math/prim/scal/meta/is_fr_var.hpp:6:
In file included from ./stan/math/prim/scal/meta/rm_complex.hpp:5:
/usr/include/c++/v1/complex:1405:9: error: no matching function for call to 'signbit'
    if (signbit(__x.imag()))
        ^~~~~~~
/usr/include/c++/v1/cmath:328:25: note: candidate template ignored: disabled by 'enable_if' [with _A1 = stan::math::zeroing<stan::math::var>]
typename std::enable_if<std::is_arithmetic<_A1>::value, bool>::type
                        ^
4 errors generated.
<builtin>: recipe for target 'test/unit/math/rev/scal/fun/acos_test.o' failed
make: *** [test/unit/math/rev/scal/fun/acos_test.o] Error 1
make -j1 test/unit/math/rev/scal/fun/acos_test failed

Although part of that error message pertains to pow, ./runTests.py test/unit/math/rev/scal/fun/pow_test.cpp actually passes.

I'll try to look at this over the next two days to see what can be done. In the meantime, please feel free to use the feature/issue-123-complex-numbers-with-vars branch on ChrisChiasson/math. I see you created a similarly named branch... I did send you an invitation to be an admin on my fork earlier this weekend, so there should't be any access issues for you.

@bob-carpenter I only have a vague sense of how the compiler is figuring (most of) this stuff out. But here is the output of git diff --name-only develop stan/ | cat, which shows the changed and added files:
stan/math/fwd/core/fvar.hpp
stan/math/fwd/core/operator_division.hpp
stan/math/fwd/cplx.hpp
stan/math/fwd/cplx/fvar.hpp
stan/math/fwd/cplx/operator_division.hpp
stan/math/fwd/mat/vectorize/apply_scalar_unary.hpp
stan/math/fwd/scal.hpp
stan/math/fwd/scal/fun/is_inf.hpp
stan/math/fwd/scal/fun/is_nan.hpp
stan/math/prim/cplx.hpp
stan/math/prim/cplx/complex.hpp
stan/math/prim/cplx/operator_addition.hpp
stan/math/prim/cplx/operator_division.hpp
stan/math/prim/cplx/operator_multiplication.hpp
stan/math/prim/cplx/operator_subtraction.hpp
stan/math/prim/cplx/zeroing.hpp
stan/math/prim/mat.hpp
stan/math/prim/mat/fun/gp_dot_prod_cov.hpp
stan/math/prim/mat/meta/Eigen_ScalarBinaryOpTraits.hpp
stan/math/prim/mat/vectorize/apply_scalar_unary.hpp
stan/math/prim/scal.hpp
stan/math/prim/scal/fun/complex_promote.hpp
stan/math/prim/scal/fun/copysign.hpp
stan/math/prim/scal/fun/grad_reg_lower_inc_gamma.hpp
stan/math/prim/scal/fun/isfinite.hpp
stan/math/prim/scal/fun/val.hpp
stan/math/prim/scal/meta/is_arith_like.hpp
stan/math/prim/scal/meta/is_complex.hpp
stan/math/prim/scal/meta/is_fr_var.hpp
stan/math/prim/scal/meta/rm_complex.hpp
stan/math/prim/scal/meta/rm_zeroing.hpp
stan/math/rev/core/std_isinf.hpp
stan/math/rev/core/std_isnan.hpp
stan/math/rev/core/var.hpp
stan/math/rev/cplx.hpp
stan/math/rev/cplx/operator_division.hpp
stan/math/rev/cplx/var.hpp
stan/math/rev/scal.hpp

So, the most important parts are in the cplx/ subdirectories of {prim,rev,fwd}. In particular, stan/math/prim/cplx/complex.hpp explains:
/*

Complex class that forwards the interface of std::complex, brining
stan::math's namespace into ADL for functions called on data types
that inherit from this class, as well as allowing template
specializations of std::complex to indirectly work with different
underlying data types. For example, std::complex specializes
the std::complex template to inherit from
stan::math::complex, which in turn inherits from
std::complex<zeroing>. This causes stan::math to be pulled into
ADL on operations involving std::complex, to handle, e.g.
division operations that the base std::complex doesn't have
defined usably for var, and, since zeroing zero-initializes
itself, will also correctly work with the remaining algorithms
in std::complex that require the expression T() to be 0.
*/

if that complex comment block above seems weird, some of the template angle brackets and their arguments disappeared on their way to html

Fixed formatting. To be fair, it is weird even when rendered properly.

I guess I had that coming, lol

Also, @bob-carpenter , there are a number of functions in prim that are only defined for doubles, such as square
https://github.com/stan-dev/math/blob/ChrisChiasson-feature/issue-123-complex-numbers-with-vars/stan/math/prim/scal/fun/square.hpp#L20
I believe the thinking was that the specializations in stan/math/rev/scal/fun/square.hpp or stan/math/fwd/scal/fun/square.hpp would be called for var and fvar<var> respectively, so the version in stan/math/prim/scal/fun/square.hpp would only get called for doubles or ints that were promoted to doubles. This logic breaks down if the thing is a std::complex<stan::math::var>, although multiplying the thing but itself should work under autodiff.
Should we change all the double only functions in prim to have a template parameter?

Usually you would not want to do that because the complex variable versions are more complicated and would waste cycles if the types were double. What we actually want to happen is that the function template overloads in std::complex catch the calls, then delegate to (sub)function calls that, when called with vars as the data type will pick up the stan implementations of the relevant (sub)functions.

Won't the compiler make separate versions of the templated function, one for double, one for std::complex<stan::math::var>, one for bool, etc.?

I am attempting to rebase your test additions tree onto this branch after an initial attempt at the fixes. There are large conflicts from the other subsystem patches that are part of the basis of your tree. Beginning to wonder if that is why you wanted me to do it, lol

Not my intent, but GitHub seems to make it difficult to add to PRs from people who are not already Stan developers. Still https://github.com/stan-dev/math/pull/915 says that it can be merged into your branch without conflicts.

After I locally committed my attempted fix, I did this git rebase origin/ChrisChiasson-feature/issue-123-complex-numbers-with-vars (stan-dev and my account both have a math repo with this branch due to github's mirroring functions ... by the way, this could be avoided by you accepting my invitation to edit my repo directly that I sent over the weekend ). That rebase went through cleanly, and at that point I am 33 commits and one different commit ahead of this PR's branch. However, rebasing onto this PR's branch with git rebase origin/feature/issue-123-complex-numbers-with-vars yields conflicts in check_matrix_gpu.hpp.

I am going to mess around with the theirs / ours switches (knowing that the meaning reverses for rebase)

I did accept your invitation but I think that was after I branched. We usually do not rebase after a branch has been pushed out, but then again, we usually are not in this situation.

Feel free to just merge instead of rebasing! Should make it a lot easier.
On Tue, Jun 26, 2018 at 2:45 PM, bgoodri<EMAIL_ADDRESS>wrote:

I did accept your invitation but I think that was after I branched. We
usually do not rebase after a branch has been pushed out, but then again,
we usually are not in this situation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-400421987, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxJ7F39G10NKiSJhIIabhKYPnqALUrCks5uAoFcgaJpZM4ShF_6
.

I'll do the rebase. Please bear with me.

Github made me do one merge due to a conflight with develop at make local (the thing about quoting boost libraries). I could have avoided it by subsequently rebasing onto develop after my other rebases, but I didn't want to go that far. Please put future complex test commits into my repo that I invited you to. This will only work if you actually try to commit to github.com/ChrisChiasson/math/feature/issue-123 ... Obviously it will not work if you try to commit to github.com/stan-dev/math/feature/issue-123 (because stan-dev doesn't have control over that branch)

acos is working on libc++ now

is something wrong with travis? it has been pending for several hours, but no tests have started

Not sure what is up with Jenkins. I ran the tests locally and the complex stuff seems to pass, but in doing so, we broke pow
In file included from test/unit/math/prim/scal/prob/frechet_ccdf_log_test.cpp:1:
In file included from ./stan/math/prim/scal.hpp:267:
In file included from ./stan/math/prim/scal/prob/frechet_ccdf_log.hpp:5:
./stan/math/prim/scal/prob/frechet_lccdf.hpp:60:36: error: no matching function for call to 'pow'
    const T_partials_return pow_ = pow(sigma_dbl / y_dbl, alpha_dbl);
                                   ^~~
test/unit/math/prim/scal/prob/frechet_ccdf_log_test.cpp:9:32: note: in instantiation of function template specialization 'stan::math::frechet_lccdf<double, double, double>' requested here
  EXPECT_FLOAT_EQ((stan::math::frechet_lccdf(y, alpha, sigma)),
                               ^
./stan/math/prim/scal/fun/pow.hpp:23:13: note: candidate template ignored: could not match 'complex<type-parameter-0-0>' against 'double'
inline auto pow(std::complex<T> const& t, U const& u) {
            ^
./stan/math/prim/scal/fun/pow.hpp:33:13: note: candidate template ignored: could not match 'complex<type-parameter-0-1>' against 'const double'
inline auto pow(T const& t, std::complex<U> const& u) {
            ^
In file included from test/unit/math/prim/scal/prob/frechet_ccdf_log_test.cpp:1:
In file included from ./stan/math/prim/scal.hpp:267:
./stan/math/prim/scal/prob/frechet_ccdf_log.hpp:16:10: error: no matching function for call to 'frechet_lccdf'
  return frechet_lccdf<T_y, T_shape, T_scale>(y, alpha, sigma);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test/unit/math/prim/scal/prob/frechet_ccdf_log_test.cpp:10:32: note: in instantiation of function template specialization 'stan::math::frechet_ccdf_log<double, double, double>' requested
      here
                  (stan::math::frechet_ccdf_log(y, alpha, sigma)));
                               ^
./stan/math/prim/scal/prob/frechet_lccdf.hpp:30:51: note: candidate template ignored: substitution failure [with T_y = double, T_shape = double, T_scale = double]
typename return_type<T_y, T_shape, T_scale>::type frechet_lccdf(
                                                  ^
2 errors generated.
<builtin>: recipe for target 'test/unit/math/prim/scal/prob/frechet_ccdf_log_test.o' failed

Yes, i was afraid of that, which is why I initially had the enable_if_ts. The trick is getting something that works on libstdc++ and libc++ and doesnt cause ambiguity errors with their different sets of template overloads. That's why I needed the tests...

ADL is common in C++. It is what makes unprefixed functions (like pow), and thus, all operators (like + or <<), call the right overloads. It works by bringing functions in the same namespace as the function or template parameter lists into the overload resolution set. I will try to answer the rest later. Your questiin about what derivatives are taken does not make any sense to me. Can you try writing the minimum math equation that would demonstrate your point? Try to expand all the complex variables i  the equation into real + i * real form so I can understand.

Right---the whole autodiff library works through argument-dependent lookup.  I'm surprised things worked before without the using std::pow that @ChrisChiasson introduced.
I can review the PR, but I might need some help on the complex arithmetic.
Is everything but the doc finished and ready to review?

I accidentally wrote that here last night on my phone. It was supposed to be in the discussion thread (I was asked if ADL is a common term used in template programming). The patch was previously ready under the reduced scope I was asking for, but now that we are adding functions I hadn't intended to, there is an issue I'm still trying to resolve with a particular invocation of std::polar under libstdc++ that happens inside another function.  I do not expect the std::polar fix (when I figure it out) to be anything more than a very subtle change... (ahh, C++, let me count the ways)

To sort of summarize for @bob-carpenter
The default constructor for a std::complex<T> has always been problematic (i.e. leaves the imaginary part undefined) when T is stan::math::var, so there is some stuff in the PR to use our own custom constructor.
There are a bunch of functions in std like conj or polar that we don't have in Stan Math because they do not make sense for integer or real numbers (irrespective of whether they are vars or fvar<var>s). But the implementation of std::conj goes in and negates the sign of the imaginary part, which has to call operator_unary_negative.hpp or whatever in Stan Math. So, there is some confusing ADL stuff in the PR to make sure implementations of functions in or outside Stan Math are internally calling Stan Math functions.
We don't actually know what are the set of functions that are defined somewhere for std::complex<T> that should or do work when T is stan::math::var, but I think the set (should) include(s) what is in the C++14 standard and what is in Eigen. Possibly it should include Boost Math but I don't think we are wrapping any complex stuff from Boost Math yet.
Anyway, for "known" functions whose signature involves a std::complex<T>, it is reasonably straight-forward to write a conventional unit test. Also, if h = 1e-8 is the imaginary part of a std::complex<stan::math::var>, then the derivative of a function with respect to the real part is equal to the imaginary part divided by h. This complex step derivative is somewhat like what Nomad did for AD. So, we can and do unit test things like std::asinh(z); that they not only have the right real part but also that the complex step derivative matches the autodiff derivative.
I think the most comprehensive test of whether it all is working correctly is test/unit/math/rev/mat/fun/complex_schur_test.cpp ,
https://github.com/stan-dev/math/pull/789/files#diff-f7e07b689c5ba35dd6fa2de8ecbbca5bR15
which passes a struct and an asymmetric matrix_d to stan::math::jacobian. The struct is called make_I and creates an identity matrix by multiplying a unitary matrix by its complex conjugate. The unitary matrix is obtained from a complex Schur decomposition of a matrix_v called A. This does real rotations to map A to H where H is almost upper triangular but has non-zero elements just below its diagonal and the offsetting rotations are applied to an identity matrix in order to form an orthogonal matrix U. So, after the first step A = U * H * U'. Then, in the second step, it casts everything to std::complex<stan::math::var> and does complex rotations to squeeze all the non-zero elements just below the diagonal of H to zero and makes the offsetting rotations to the unitary matrix U. This is an iterative process and terminates when the below diagonal elements are sufficiently close to zero, leaving H as a complex triangular matrix. We pre-multiply the resulting U by its conjugate transpose to create M and then add the imaginary parts of M (which should all be zero) to the real parts of M (which should all be zero except for ones on the diagonal) to create the matrix_v I and return that. Up to numerical tolerance, this actually is an identity matrix and the Jacobian is a zero matrix. This one test has many Stan Math functions in its AST:
Filename                                                                                Regions    Miss   Cover Functions  Executed
------------------------------------------------------------------------------------------------------------------------------------
/home/ben/stan-dev/math/./stan/math/memory/stack_alloc.hpp                                    40      18  55.00%        14    50.00%
/home/ben/stan-dev/math/./stan/math/prim/cplx/complex.hpp                                      5       1  80.00%         5    80.00%
/home/ben/stan-dev/math/./stan/math/prim/cplx/operator_division.hpp                            3       0 100.00%         3   100.00%
/home/ben/stan-dev/math/./stan/math/prim/scal/fun/hypot.hpp                                    2       1  50.00%         2    50.00%
/home/ben/stan-dev/math/./stan/math/prim/scal/fun/is_inf.hpp                                   1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/prim/scal/fun/is_nan.hpp                                   1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/prim/scal/fun/signbit.hpp                                  2       1  50.00%         2    50.00%
/home/ben/stan-dev/math/./stan/math/prim/scal/fun/val.hpp                                      5       2  60.00%         5    60.00%
/home/ben/stan-dev/math/./stan/math/rev/core/autodiffstackstorage.hpp                          1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/empty_nested.hpp                                  1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/gevv_vvv_vari.hpp                                16       1  93.75%         4    75.00%
/home/ben/stan-dev/math/./stan/math/rev/core/grad.hpp                                          6       1  83.33%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/nested_size.hpp                                   1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_addition.hpp                            11       5  54.55%         7    42.86%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_division.hpp                            13       7  46.15%         9    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_equal.hpp                                3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_greater_than.hpp                         3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_greater_than_or_equal.hpp                3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_less_than.hpp                            3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_less_than_or_equal.hpp                   3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_minus_equal.hpp                          2       1  50.00%         2    50.00%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_multiplication.hpp                      18      12  33.33%         7    42.86%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_multiply_equal.hpp                       2       1  50.00%         2    50.00%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_not_equal.hpp                            3       2  33.33%         3    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_plus_equal.hpp                           2       1  50.00%         2    50.00%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_subtraction.hpp                         13       7  46.15%         9    33.33%
/home/ben/stan-dev/math/./stan/math/rev/core/operator_unary_negative.hpp                       6       1  83.33%         3   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/recover_memory_nested.hpp                         7       3  57.14%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/set_zero_all_adjoints_nested.hpp                 14       5  64.29%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/start_nested.hpp                                  1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/std_isinf.hpp                                     2       0 100.00%         2   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/std_isnan.hpp                                     2       0 100.00%         2   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/std_numeric_limits.hpp                            8       6  25.00%         8    25.00%
/home/ben/stan-dev/math/./stan/math/rev/core/v_vari.hpp                                        1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/core/var.hpp                                          24      18  25.00%        24    25.00%
/home/ben/stan-dev/math/./stan/math/rev/core/vari.hpp                                          8       3  62.50%         8    62.50%
/home/ben/stan-dev/math/./stan/math/rev/core/vv_vari.hpp                                       1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/cplx/operator_division.hpp                             1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/mat/fun/Eigen_NumTraits.hpp                           20       8  60.00%        10    40.00%
/home/ben/stan-dev/math/./stan/math/rev/mat/functor/jacobian.hpp                              18       1  94.44%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/abs.hpp                                       1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/atan2.hpp                                     9       6  33.33%         9    33.33%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/cos.hpp                                       3       0 100.00%         3   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/fabs.hpp                                     10       1  90.00%         1   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/pow.hpp                                      34      27  20.59%         9    11.11%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/sin.hpp                                       3       0 100.00%         3   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/sqrt.hpp                                      3       0 100.00%         3   100.00%
/home/ben/stan-dev/math/./stan/math/rev/scal/fun/square.hpp                                    3       0 100.00%         3   100.00%
/home/ben/stan-dev/math/lib/gtest_1.7.0/src/gtest_main.cc                                      1       0 100.00%         1   100.00%
/home/ben/stan-dev/math/test/unit/math/rev/mat/fun/complex_schur_test.cpp                     25       0 100.00%         2   100.00%

Our complex constructor is being called by this test ~28 thousand times
       |   38|   * tparam R type of real argument. Won't fire on non-arithmetic types.
       |   39|   * tparam I type of imaginary argument.
       |   40|   * param[in] real, the pure real component of the complex number.
       |   41|   * param[in] imag, the pure imaginary component of the complex number
       |   42|   */
       |   43|  template <class R = double, class I = double,
       |   44|            std::enable_if_t<is_arith_like<R>::value>* = nullptr>
       |   45|  // NOLINTNEXTLINE(runtime/explicit)
       |   46|  complex(R const& real = 0, I const& imag = 0)
  28.2k|   47|      : std::complex<zeroing<T>>(real, imag) {}
  ------------------

and it seems to be doing stuff with those ~28 thousand std::complex<stan::math::var> values that constructs the AD expression tree correctly. What we (or at least I) don't know is how the compiler manages to figure out how to do all that.

So, old g++ is not letting my function template overload for std::polar work in one of the sub function calls in the new pow_test, but current g++ does. I will try to figure out what's up.

You only need to worry about g++ >= 4.9.3
On Thu, Jun 28, 2018 at 5:28 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

So, old g++ is not letting my function template overload for std::polar
work in one of the sub function calls in the new pow_test, but current g++
does. I will try to figure out what's up.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401178982, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqvgQO1ERI6NMUe3VjXskBv9ILdh2ks5uBUp4gaJpZM4ShF_6
.

I think that's what I'm running on 16.04 LTS. I am logged into my 18.04 server VM now...

If I do review this, the first thing I'm going to insist on is some delineation of scope so that I know what needs to be tested.  The goal "support complex" numbers isn't enough, and while "build something so some Eigen functions that use std::complex internally will work" is better, it doesn't say which functions will work.
About all I know about complex numbers is that sqrt(-1) = i.

how the compiler manages to figure out how to do all that.

Is there something specific that's confusing about ADL or something else?
Daniel Lee also asked me to review the template metaprograms to see if they could be consolidated with our other metaprograms.

I would say "almost any Eigen function that is defined for Eigen::Matrix<T,
Eigen::Dynamic, Eigen::Dynamic>", but there are not any specific Eigen
functions being exposed in this PR. In the unit tests, we have

Eigenvalues / vectors of an asymmetric real matrix
Complex Schur decomposition of an asymmetric real matrix
Pseudo-eigenvalues/vectors of an asymmetric real matrix

The Eigen functions that are said to not work are those in the unsupported
module, whose doc says the matrix has to be "has to be a matrix of float,
double, long double complex, complex, or complex", which is why we had to write / copy our own implementation of the
matrix exponential to avoid using std::ldexp.
But what does listing out the Eigen functions, like FFT, that we might want
to wrap in future PRs accomplish? For any particular Eigen function, we can
write a test to verify that it yields the expected answer. We don't
understand how the compiler gets it to work, so it is difficult to know
when we should expect in advance whether something will work, compile but
not work, or not compile.
On Thu, Jun 28, 2018 at 5:34 PM Bob Carpenter<EMAIL_ADDRESS>wrote:

If I do review this, the first thing I'm going to insist on is some
delineation of scope so that I know what needs to be tested. The goal
"support complex" numbers isn't enough, and while "build something so some
Eigen functions that use std::complex internally will work" is better, it
doesn't say which functions will work.
About all I know about complex numbers is that sqrt(-1) = i.

how the compiler manages to figure out how to do all that.

Is there something specific that's confusing about ADL or something else?
Daniel Lee also asked me to review the template metaprograms to see if
they could be consolidated with our other metaprograms.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401180593, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqmZx79yVNqkEg3LS2kcR06-JsNzYks5uBUv0gaJpZM4ShF_6
.

In other words, I don't think it is that much of a question about particular functions. The question is whether it is a correctly implemented extension on Stan Math's autodiff to the complex plane for any function that is complex differentiable?

There are two main reasons why it works...

template specializations don't necessarily have anything to do with the original class template
ADL brings in namespaces of function parameters, template parameters, and all associated base classes when trying to look up unqualified function names.

From number 1, we get that a full template specialization for something like std::complex<var> can be made to inherit from a special class, stan's complex<var>, that in turn inherits from the original std::complex template with an appropriate type like zeroing<var> that can survive value-initialization [aka zeroing<var>()] in a ready to operate state.
From number 2, the std::complex specialization will cause ADL to bring in the relevant stan namespace operators for complex types (+, -, *, /, pow, etc...). Inside the complex functions, when they, in turn, delegate to non-complex functions because they are handling the individual real or imaginary parts, normal stan ADL takes over. Extension of complex constructors is handled within stan's complex.

"There are two main reasons why it works" That is what I was getting at. We need someone to review whether those two things logically imply the correctness of complex AD, assuming Stan's existing AD is correct. And to review whether the PR actually accomplishes those two things. Those are almost entirely C++ questions, rather than questions about complex numbers. Knowing that std::acosh works for a complex var or that some Eigen function works on a matrix of complex vars isn't a surprise if the above is true but seeing them work in unit tests only goes part way toward showing the above is true.

I would say that it must work under those assumptions. A sufficient representation of a complex number is a real number plus sqrt(-1) times another real number. Since the complex template class handles everything involving the sqrt(-1) constant manually (i.e. always working with the expanded form: x + i y), the only manipulations left over are those of the real number data types. Stan is designed to chain operations on real numbers correctly, so it follows that the tracking of derivative information through the real numbers will be correct.

We do need to get it to work on Windows. I'll try to fix it.
On Fri, Jun 29, 2018 at 2:27 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

I thought I fixed it this morning, but I remembered over lunch that the
problem was on g++-4.9, not old clang.
This test program illustrates the problem. Note that on current g++, old
clang, and current clang, this works fine.
#include <stan/math/rev/scal.hpp>
#include
int main(int,char*[]){
using stan::math::var; using stan::math::zeroing;
std::cout<<std::polar(var(1),zeroing(1))<<"\n"; // (0.540302,0.841471)
std::complex i(0, 1);
std::cout<<std::polar(exp(i.real()),i.imag())<<"\n"; // (0.540302,0.841471)
// g++-4.9: next line fails calling std::polar(var,zeroing)
std::complex f(pow(i, i));
std::cout<<f<<"\n";  // (0.20788,0) on compilers that work
}
I am starting to wonder if maybe I should just g++ compiler
old-version-ifdef-out the relevant line of pow_test. The reason why is that
these old compilers are only used because of stan's interfaces to programs
like R stan that require old compilers. Since none of the complex functions
are exposed to R stan... the reason for working on those old compilers
disappears.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401436420, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqljZlQ3AnfNsYHzU4S9rHM7gGU1_ks5uBnGZgaJpZM4ShF_6
.

What's on Windows that requires complex math and g++-4.9?

The Rtools distribution for Windows includes g++ 4.9, which is the main way users install for Windows (not the only option, just the most common for R users). So if things don't compile under g++ 4.9, we lose something like half our users.

On Jun 29, 2018, at 2:41 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
What's on Windows that requires complex math and g++-4.9?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

It would compile. The ifdef would only be in the test, and the function would never be called.

Wasn't the purpose to allow us to eventually call functions that need std::complex?
I believe the plan is that the R project is going to update Rtools at some point during the summer and RStudio's going to make it much easier to install.  I believe we're trying to figure out how to cut that dependency to the Rtools compiler at the same time while still supporting R users on Windows.

On Jun 29, 2018, at 2:46 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
It would compile. The ifdef would only be in the test, and the function would never be called.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Wouldn't it work if we defined our own polar function in
stan/math/rev/scal/fun that called std::polar for g++ >= 5.0 and
implemented it from the definition otherwise?
On Fri, Jun 29, 2018 at 2:46 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

It would compile. The ifdef would only be in the test, and the function
would never be called.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401441374, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqjTxoSa6b7WNcCg7yg1W_X1j6bRUks5uBnYHgaJpZM4ShF_6
.

It's in there already :-(
On Fri, Jun 29, 2018, 1:56 PM bgoodri<EMAIL_ADDRESS>wrote:

Wouldn't it work if we defined our own polar function in
stan/math/rev/scal/fun that called std::polar for g++ >= 5.0 and
implemented it from the definition otherwise?
On Fri, Jun 29, 2018 at 2:46 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

It would compile. The ifdef would only be in the test, and the function
would never be called.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401441374, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ADOrqjTxoSa6b7WNcCg7yg1W_X1j6bRUks5uBnYHgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401443903, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-68SF2uxnfWNWfHSQJ1UIJPWQRRvks5uBnhdgaJpZM4ShF_6
.

I am only seeing an implementation of polar in stan/math/prim/scal/fun and
it does not branch on the compiler version.
On Fri, Jun 29, 2018 at 3:09 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

It's in there already :-(
On Fri, Jun 29, 2018, 1:56 PM bgoodri<EMAIL_ADDRESS>wrote:

Wouldn't it work if we defined our own polar function in
stan/math/rev/scal/fun that called std::polar for g++ >= 5.0 and
implemented it from the definition otherwise?
On Fri, Jun 29, 2018 at 2:46 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

It would compile. The ifdef would only be in the test, and the function
would never be called.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401441374, or
mute
the thread
<

https://github.com/notifications/unsubscribe-auth/ADOrqjTxoSa6b7WNcCg7yg1W_X1j6bRUks5uBnYHgaJpZM4ShF_6

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401443903, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ALTv-68SF2uxnfWNWfHSQJ1UIJPWQRRvks5uBnhdgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401447225, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqsrjL6tkVdAAAyqZZFUf7ykLqsGlks5uBnthgaJpZM4ShF_6
.

Correct. The only part that needs to branch is the test that eventually makes the call (what happens is that g++-4.9 does not find the overload while inside of the complex header, but does find the overload when it is elsewhere. The function definition itself won't cause a compile error. This is most likely due to rules about what definitions are available to template instantiations ... g++-4.9 probably has one of those rules written incorrectly.

I can replicate the compiler error:
/usr/include/c++/4.9/complex: In instantiation of ‘std::complex<_Tp> std::__complex_exp(const std::complex<_Tp>&) [with _Tp = stan::math::zeroing<stan::math::var>]’:
/usr/include/c++/4.9/complex:753:68:   required from ‘std::complex<_Tp> std::exp(const std::complex<_Tp>&) [with _Tp = stan::math::zeroing<stan::math::var>]’
/usr/include/c++/4.9/complex:1004:65:   required from ‘std::complex<_Tp> std::__complex_pow(const std::complex<_Tp>&, const std::complex<_Tp>&) [with _Tp = stan::math::zeroing<stan::math::var>]’
/usr/include/c++/4.9/complex:1023:52:   required from ‘std::complex<_Tp> std::pow(const std::complex<_Tp>&, const std::complex<_Tp>&) [with _Tp = stan::math::var]’
test/unit/math/rev/scal/fun/polar_test.cpp:15:30:   required from here
/usr/include/c++/4.9/complex:738:52: error: no matching function for call to ‘polar(stan::math::var, stan::math::zeroing<stan::math::var>)’
     { return std::polar(exp(__z.real()), __z.imag()); }
                                                    ^
/usr/include/c++/4.9/complex:738:52: note: candidate is:
/usr/include/c++/4.9/complex:664:5: note: template<class _Tp> std::complex<_Tp> std::polar(const _Tp&, const _Tp&)
     polar(const _Tp& __rho, const _Tp& __theta)
     ^
/usr/include/c++/4.9/complex:664:5: note:   template argument deduction/substitution failed:
/usr/include/c++/4.9/complex:738:52: note:   deduced conflicting types for parameter ‘const _Tp’ (‘stan::math::var’ and ‘stan::math::zeroing<stan::math::var>’)
     { return std::polar(exp(__z.real()), __z.imag()); }                                                    ^

and that it does not happen with g++-5.
But I don't understand what branching on the test would accomplish other than the tests compiling. If someone using g++-4.9 (i.e. on Windows) writes a program that tries to call polar(x, y) with x and y being std::complex<stan::math::var>, then it will fail to compile, right?

It would only fail to compile if the arguments were like (var, zeroing) if they were the same, it would still compile and return the correct answer (because the template in complex would handle it)... and even then, it would only happen on g++-4.9 and only if you were actually using complex math.

by the way, it only happens if the call from inside complex, which is why it happens on the 3rd part of the example rather than the first two parts

Hrm, maybe I could attempt to overload the containing function of the relevant std::polar call.

Does that mean specialize exp in the case that its input is zeroing ?
On Fri, Jun 29, 2018 at 5:33 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

Hrm, maybe I could attempt to overload the containing function of the
relevant std::polar call.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401479809, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqv5ySWGbret8POnZqd4UOPKuGwEsks5uBp1AgaJpZM4ShF_6
.

I was thinking about it (exp), but I would have to be careful that I had encompassed all functions that call into std::polar in that way. Also, there are other ways I could attempt to trigger the right behavior (explicitly written overloads, template specializations, etc), but it is difficult to know where the bug begins and ends without experimentation.

My other idea was to try to make zeroing a constexpr .
On Fri, Jun 29, 2018 at 5:44 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

I was thinking about it (exp), but I would have to be careful that I had
encompassed all functions that call into std::polar in that way. Also,
there are other ways I could attempt to trigger the right behavior
(explicitly written overloads, template specializations, etc), but it is
difficult to know where the bug begins and ends without experimentation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401481865, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqkLIPHtHb3tXAMCjXnABOW8LSvg-ks5uBp_VgaJpZM4ShF_6
.

You lost me on that one (?). I'm going to try shadowing the implementation of the real and imag member functions via stan's complex middleware to force them to return variables of type T rather than zeroing.

Okay, try your way first. I was thinking maybe we could convince the
compiler to resolve stan::math::zeroingstan::math::var to
stan::math::var(0.0) at compile time rather than runtime using a constrexpr
in which case it would not think the template types of the real and
imaginary parts conflict.
On Fri, Jun 29, 2018 at 5:55 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

You lost me on that one (?). I'm going to try shadowing the implementation
of the real and imag member functions via stan's complex middleware to
force them to return variables of type T rather than zeroing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401483774, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqpbiPUPZOqy69cLEkwqMYPKZdZk4ks5uBqI9gaJpZM4ShF_6
.

Also, with zeroing.hpp defined as
zeroing(T const& t = 0) : T(t) {}  // NOLINT(runtime/explicit)
what if something calls zeroingstan::math::var(42) ? Should it be :
T(0.0) ?
On Fri, Jun 29, 2018 at 6:00 PM Ben Goodrich<EMAIL_ADDRESS>wrote:

Okay, try your way first. I was thinking maybe we could convince the
compiler to resolve stan::math::zeroingstan::math::var to
stan::math::var(0.0) at compile time rather than runtime using a constrexpr
in which case it would not think the template types of the real and
imaginary parts conflict.
On Fri, Jun 29, 2018 at 5:55 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

You lost me on that one (?). I'm going to try shadowing the
implementation of the real and imag member functions via stan's complex
middleware to force them to return variables of type T rather than zeroing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401483774, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqpbiPUPZOqy69cLEkwqMYPKZdZk4ks5uBqI9gaJpZM4ShF_6
.

Shadowing real() and imag() was a bust; by the time that it gets down to the relevant calls, what has been passed in doesn't view the complex through stan's middle-ware. I looked at other call sites of std::polar within complex (too bad they didn't use unprefixed polar... then we likely wouldn't have this problem because we could rely on ADL)... Some of them occur in functions that take more than one argument. Maybe we should just patch exp and see if any of the other problems turn up in practice?

if the zeroing<T> ctor init list entry for the base class had T(0) rather than T(t), then when the std::complex<T> class called T(42), it would get a 0 when it wanted a 42

OK, I thought zeroing was always supposed to return zero instead of zero by
default. In that case, it does not meet the definition of a constexpr.
Does this help?
https://stackoverflow.com/questions/21737186/how-to-use-complex-numbers-with-boostmultiprecisionmpfr
Not that we want to use boost::multiprecision but it discusses the same
problem.
On Fri, Jun 29, 2018 at 6:43 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

if the zeroing ctor init list entry for the base class had T(0) rather
than T(t), then when the std::complex class called T(42), it would get
a 0 when it wanted a 42
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401491716, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqiIsTQtQidtXMV9XEpTqtgUaZkLrks5uBq2hgaJpZM4ShF_6
.

That link you sent contains a link to a g++ bug report, with a proposed fix that would explain why higher gcc versions work. The stack overflow article said the patch would be in version 4.10.
In the stack overflow answer, they say that it seems like ADL is broken, but actually something else is broken, because the correct overload isn't being used even when the correct namespace is taken.
Great find. It also may explain why my attempt at patching std::exp isn't working either (and yes, there is at least one std::exp prefixed call in complex).
At this point, the next stop may be to attempt to either comment out the test on g++-4.9, or start generating top level function templates for pow that completely avoid these function pathways (if that will even work - we don't know how far this bug goes...)
I wonder if it is possible to modify the macro called out in the stack overflow article so that it doesn't prevent g++ from working.

Putting
#define _GLIBCXX_USE_C99_COMPLEX 0
at the top of the test file does not seem to help any.
I guess if it is a g++-4.9 bug that we cannot work around, we should just
detect the compiler and put a static_assert in the relevant Stan functions
explaining to use a different compiler.
On Fri, Jun 29, 2018 at 7:10 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

That link you sent contains a link to a g++ bug report, with a proposed
fix that would explain why higher gcc versions work. The stack overflow
article said the patch would be in version 4.10.
In the stack overflow answer, they say that it seems like ADL is broken,
but actually something else is broken, because the correct overload isn't
being used even when the correct namespace is taken.
Great find. It also may explain why my attempt at patching std::exp isn't
working either (and yes, there is at least one std::exp prefixed call in
complex).
At this point, the next stop may be to attempt to either comment out the
test on g++-4.9, or start generating top level function templates for pow
that completely avoid these function pathways (if that will even work - we
don't know how far this bug goes...)
I wonder if it is possible to modify the macro called out in the stack
overflow article so that it doesn't prevent g++ from working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401495492, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqvBjzG5THpWuDcL4n10w4_EEkO9pks5uBrPSgaJpZM4ShF_6
.

If I define
inline std::complexstan::math::var
pow(const std::complexstan::math::var& base,
const std::complexstan::math::var& exponent) {
return exp(exponent * log(base));
}
in stan/math/rev/scal/fun/pow.hpp, then the compiler finds this
specialization but still fails with the same compiler error when it calls
exp. If I return some constant, then the test runs but the answer is
meaningless.
On Fri, Jun 29, 2018 at 7:53 PM Ben Goodrich<EMAIL_ADDRESS>wrote:

Putting
#define _GLIBCXX_USE_C99_COMPLEX 0
at the top of the test file does not seem to help any.
I guess if it is a g++-4.9 bug that we cannot work around, we should just
detect the compiler and put a static_assert in the relevant Stan functions
explaining to use a different compiler.
On Fri, Jun 29, 2018 at 7:10 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

That link you sent contains a link to a g++ bug report, with a proposed
fix that would explain why higher gcc versions work. The stack overflow
article said the patch would be in version 4.10.
In the stack overflow answer, they say that it seems like ADL is broken,
but actually something else is broken, because the correct overload isn't
being used even when the correct namespace is taken.
Great find. It also may explain why my attempt at patching std::exp isn't
working either (and yes, there is at least one std::exp prefixed call in
complex).
At this point, the next stop may be to attempt to either comment out the
test on g++-4.9, or start generating top level function templates for pow
that completely avoid these function pathways (if that will even work - we
don't know how far this bug goes...)
I wonder if it is possible to modify the macro called out in the stack
overflow article so that it doesn't prevent g++ from working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401495492, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqvBjzG5THpWuDcL4n10w4_EEkO9pks5uBrPSgaJpZM4ShF_6
.

the problem is that fvar will still need a template specified and I already
tried a template based on zeroing... Of course we could try to enumerate
all the fvar types that we think are likely
On Fri, Jun 29, 2018, 7:18 PM bgoodri<EMAIL_ADDRESS>wrote:

If I define
inline std::complexstan::math::var
pow(const std::complexstan::math::var& base,
const std::complexstan::math::var& exponent) {
return exp(exponent * log(base));
}
in stan/math/rev/scal/fun/pow.hpp, then the compiler finds this
specialization but still fails with the same compiler error when it calls
exp. If I return some constant, then the test runs but the answer is
meaningless.
On Fri, Jun 29, 2018 at 7:53 PM Ben Goodrich<EMAIL_ADDRESS>wrote:

Putting
#define _GLIBCXX_USE_C99_COMPLEX 0
at the top of the test file does not seem to help any.
I guess if it is a g++-4.9 bug that we cannot work around, we should just
detect the compiler and put a static_assert in the relevant Stan
functions
explaining to use a different compiler.
On Fri, Jun 29, 2018 at 7:10 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

That link you sent contains a link to a g++ bug report, with a proposed
fix that would explain why higher gcc versions work. The stack overflow
article said the patch would be in version 4.10.
In the stack overflow answer, they say that it seems like ADL is broken,
but actually something else is broken, because the correct overload
isn't
being used even when the correct namespace is taken.
Great find. It also may explain why my attempt at patching std::exp
isn't
working either (and yes, there is at least one std::exp prefixed call in
complex).
At this point, the next stop may be to attempt to either comment out the
test on g++-4.9, or start generating top level function templates for
pow
that completely avoid these function pathways (if that will even work -
we
don't know how far this bug goes...)
I wonder if it is possible to modify the macro called out in the stack
overflow article so that it doesn't prevent g++ from working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401495492, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ADOrqvBjzG5THpWuDcL4n10w4_EEkO9pks5uBrPSgaJpZM4ShF_6

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401503448, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-9HZ58vWut56jMTZChK2negpUlHNks5uBsPugaJpZM4ShF_6
.

Should we try to undefine the macro?
On Fri, Jun 29, 2018, 7:46 PM Chris Chiasson<EMAIL_ADDRESS>wrote:

the problem is that fvar will still need a template specified and I
already tried a template based on zeroing... Of course we could try to
enumerate all the fvar types that we think are likely
On Fri, Jun 29, 2018, 7:18 PM bgoodri<EMAIL_ADDRESS>wrote:

If I define
inline std::complexstan::math::var
pow(const std::complexstan::math::var& base,
const std::complexstan::math::var& exponent) {
return exp(exponent * log(base));
}
in stan/math/rev/scal/fun/pow.hpp, then the compiler finds this
specialization but still fails with the same compiler error when it calls
exp. If I return some constant, then the test runs but the answer is
meaningless.
On Fri, Jun 29, 2018 at 7:53 PM Ben Goodrich<EMAIL_ADDRESS>wrote:

Putting
#define _GLIBCXX_USE_C99_COMPLEX 0
at the top of the test file does not seem to help any.
I guess if it is a g++-4.9 bug that we cannot work around, we should
just
detect the compiler and put a static_assert in the relevant Stan
functions
explaining to use a different compiler.
On Fri, Jun 29, 2018 at 7:10 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

That link you sent contains a link to a g++ bug report, with a proposed
fix that would explain why higher gcc versions work. The stack overflow
article said the patch would be in version 4.10.
In the stack overflow answer, they say that it seems like ADL is
broken,
but actually something else is broken, because the correct overload
isn't
being used even when the correct namespace is taken.
Great find. It also may explain why my attempt at patching std::exp
isn't
working either (and yes, there is at least one std::exp prefixed call
in
complex).
At this point, the next stop may be to attempt to either comment out
the
test on g++-4.9, or start generating top level function templates for
pow
that completely avoid these function pathways (if that will even work

we

don't know how far this bug goes...)
I wonder if it is possible to modify the macro called out in the stack
overflow article so that it doesn't prevent g++ from working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401495492,
or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ADOrqvBjzG5THpWuDcL4n10w4_EEkO9pks5uBrPSgaJpZM4ShF_6

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401503448, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-9HZ58vWut56jMTZChK2negpUlHNks5uBsPugaJpZM4ShF_6
.

If I put this at the end of stan/math/rev/scal/fun/pow.hpp
inline std::complexstan::math::var
pow(const std::complexstan::math::var& base,
const std::complexstan::math::var& exponent) {
auto z = exponent * log(base);
auto a = real(z);
auto b = imag(z);
return exp(a) * std::complexstan::math::var(cos(b), sin(b));
}
and
#include <stan/math/rev/scal/fun/cos.hpp>
#include <stan/math/rev/scal/fun/exp.hpp>
#include <stan/math/rev/scal/fun/sin.hpp>
at the top, then the test has the right output with g++-4.9:
(0.540302,0.841471)
(0.540302,0.841471)
(0.20788,0)
Is that going to mess something else up and / or do we need to do similar
things elsewhere?

We would need to do similar things for all likely template parameters of
fvar (that was my question at the end of the last message... is it
acceptable to do it that way, because fvar is technically a template, and T
could be anything)
On Fri, Jun 29, 2018, 8:29 PM bgoodri<EMAIL_ADDRESS>wrote:

If I put this at the end of stan/math/rev/scal/fun/pow.hpp
inline std::complexstan::math::var
pow(const std::complexstan::math::var& base,
const std::complexstan::math::var& exponent) {
auto z = exponent * log(base);
auto a = real(z);
auto b = imag(z);
return exp(a) * std::complexstan::math::var(cos(b), sin(b));
}
and
#include <stan/math/rev/scal/fun/cos.hpp>
#include <stan/math/rev/scal/fun/exp.hpp>
#include <stan/math/rev/scal/fun/sin.hpp>
at the top, then the test has the right output with g++-4.9:
(0.540302,0.841471)
(0.540302,0.841471)
(0.20788,0)
Is that going to mess something else up and / or do we need to do similar
things elsewhere?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401508336, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-8Iw3U18fLmLMkfr4v4ttlDKFQ8wks5uBtRhgaJpZM4ShF_6
.

That is a question for @bob-carpenter

I know I dont get a vote, but my two cents: windows R stan users that
suddenly decide to start programming C++ complex math involving these
specific operations (rather than whatever eigen is doing on those matrix
decompositions) can install a later version of gcc... I don't like the
maintence burden of generating a lot of different code paths to check
On Fri, Jun 29, 2018, 8:39 PM bgoodri<EMAIL_ADDRESS>wrote:

That is a question for @bob-carpenter https://github.com/bob-carpenter
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401508878, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-9uwGT8sKt8MXexrs4jZIIal5A4uks5uBtbBgaJpZM4ShF_6
.

The Eigen doc says to define things like exp and pow for custom scalar
types in order to be used with Eigen
http://eigen.tuxfamily.org/dox/TopicCustomizing_CustomScalar.html
Hopefully, those are the only two problematic ones.

Thanks a lot for sticking with this.  We're usually very safety oriented for the code, but not this picky about PRs.  This one seems problematic for compatibility and is also touching our very core inner-loop code.  So please bear with us.  We do want to get this in.

On Jun 29, 2018, at 9:43 PM, ChrisChiasson<EMAIL_ADDRESS>wrote:
I know I dont get a vote,

I don't, either.  Nobody does.  Daniel Lee gets the final decision in cases of lack of consensus for the math lib.  At the moment, there is no higher-level arbiter in cases of disagreements among module maintainers.
We're working on a better governance structure, but it's a slow process, as there are dozens of stakeholders.
Making these decisions is a lot easier when the goal of the PR is clear.  So let me try to summarize my understanding of the goal:

implement enough of std::complex that we can autodiff through Eigen implementations of real-valued functions that use complex numbers under the hood (e.g., FFT).

support math library clients that want to use complex computations (e.g., Chris Chiasson)

The best place to clarify the intent is in the issue that the PR solves.

but my two cents: windows R stan users that
suddenly decide to start programming C++ complex math involving these
specific operations (rather than whatever eigen is doing on those matrix
decompositions) can install a later version of gcc...

At least in the short term, complex numbers won't be exposed in the Stan language, so they're encapsulated for ordinary RStan users.  This is the bit that should work everywhere.
RStan contains a library for extraordinary users (complement of ordinary users?) allowing C++ to be linked and the language to be extended.  I'm OK having that be limited on some platforms.

I don't like the
maintence burden of generating a lot of different code paths to check

Nobody does.  Sean's been talking about limiting our compiler dependencies.  Our current architecture requires linking with Python and R, which is a huge pain to support cross-platform, particularly on Windows.

@bob-carpenter Is it okay to define a few functions (like pow) to preempt those in std that are buggy under g++-4.9? This is easier for rev but more involved for fwd because we would need an implementation for everything that fvar<T> could be. But I guess we are not really too concerned about people trying to instantiate a fvar<char> or anything like that.

What if we restricted specific implementations for var, fvar, and
fvar and put a condition around the definitions so clang and new gcc
would not use them? Then we could be sure the general template code is
being tested by clang and that the specific functions are tested by old gcc
until such time as we move off old gcc? I think bgoodri made a good effort
to solve this one and I would hate to see it go to waste.
On Sun, Jul 1, 2018, 11:58 AM bgoodri<EMAIL_ADDRESS>wrote:

@bob-carpenter https://github.com/bob-carpenter Is it okay to define a
few functions (like pow) to preempt those in std that are buggy under
g++-4.9? This is easier for rev but more involved for fwd because we
would need an implementation for everything that fvar could be. But I
guess we are not really too concerned about people trying to instantiate a
fvar or anything like that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401619051, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-9rwuwkRkzsdDiGojgDRtC-lC_geks5uCP-ogaJpZM4ShF_6
.

I was thinking about #undef _GLIBCXX_USE_C99_COMPLEX ... it would be hard to enumerate all the #include directives. For instance, we would have to prevent loading of Eigen until after #undef, and all stan complex includes would need to be routed into a file that does the #undef first.

I was thinking
#if GNUC == 4 && GNUC_MINOR == 9
// define pow, etc.
#endif
On Sun, Jul 1, 2018 at 2:07 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

I was thinking about #undef _GLIBCXX_USE_C99_COMPLEX ... it would be hard
to enumerate all the #include directives. For instance, we would have to
prevent loading of Eigen until after #undef, and all stan complex includes
would need to be routed into a file that does the #undef first.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401622978, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqnG_V20QlOOPXhezA-jPHD8782SMks5uCQ_ngaJpZM4ShF_6
.

By the way, the overloads could can from polar since the problem is only with template overloads: std::polar(var r, var th) {return std::polar(r, th);}. That way, we cover all three call sites of std::polar inside complex by overloading just one function, and the definition of the overloads will be simple because it would just forward both args....

That seems less invasive than undefing the C99 standard

The macro is just to control whether the implementation uses hard coded
functions or C++ templates for complex math. I am not sure why the people
on the stack overflow thread are saying it is the culprit, but on the off
chance that it is... I was just musing about whether it would be possible
to undefine it
On Sun, Jul 1, 2018, 6:31 PM bgoodri<EMAIL_ADDRESS>wrote:

That seems less invasive than undefing the C99 standard
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401640726, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTv-9TmaU1RzOL02aHqcTdPOZY_vFjVks5uCVvXgaJpZM4ShF_6
.

Ok, let's try with your polar overload strategy.
On Sun, Jul 1, 2018 at 7:35 PM ChrisChiasson<EMAIL_ADDRESS>wrote:

The macro is just to control whether the implementation uses hard coded
functions or C++ templates for complex math. I am not sure why the people
on the stack overflow thread are saying it is the culprit, but on the off
chance that it is... I was just musing about whether it would be possible
to undefine it
On Sun, Jul 1, 2018, 6:31 PM bgoodri<EMAIL_ADDRESS>wrote:

That seems less invasive than undefing the C99 standard
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401640726, or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/ALTv-9TmaU1RzOL02aHqcTdPOZY_vFjVks5uCVvXgaJpZM4ShF_6
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/789#issuecomment-401640902, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADOrqpVk4cnBl7QfueUk4NK1VrGUP9aSks5uCVzAgaJpZM4ShF_6
.

ok, on it

So I created all the files, thinking everything was going to work... but it didn't. I guess the reason why is that ordinary functions need to be declared before they are used inside the template in complex (otherwise they aren't seen). Your function with pow avoids the problem because it isn't called from within complex -- it is called with the client code. In order to make mine work, I'd have to get it out ahead of the very first complex include in the translation unit...

(by the way, because complex is necessarily included in prim, I can't put the polar function definitions ahead of the complex include... the best I could do would be to forward declare them, which is verboten due to the data types they use)

Did you try make test-headers with the pow overload version? I had added a templated function called pow_helper in the prim pow source file, and then included that from the rev pow complex overload (an overload I guarded so only GCC would pick it up since clang brings in more functions). When I did this, my includes look like the following, but of course it generates problems with make test-headers.
#define STAN_MATH_REV_SCAL_FUN_POW_HPP

#include <stan/math/rev/cplx.hpp>  // gcc 4.9 cplx
#include <stan/math/rev/core.hpp>
#include <stan/math/rev/scal/fun/cos.hpp>  // gcc 4.9 cplx
#include <stan/math/rev/scal/fun/exp.hpp>  // gcc 4.9 cplx