Complex number support in Math library autodiff

#55

@ChrisChiasson: thanks. Sorry, I’m not trying to be antagonistic. I just don’t understand what’s going on. More below.

Thanks. I missed it when I looked at the code, probably because it was named complexNotNullIssue123. I had assumed the test was testing that things weren’t null.

One thing that would help is to not use auto in this test. I’m going to assume abs(std:complex<T>) returns T. And then autodiff holds.

The question I have is… are there any situations where if you had a std::complex<stan::math::var> x, would you ever want the gradients of both the real and imaginary components? That’s these two things: x.real().grad(d_real) and x.imag().grad(d_imag)? Or… will you ever only want gradients after a function that takes a complex value and returns something on the real plane?

Yes.

What is “termination”? I haven’t seen that term used in conjunction with ADL. Is this called something else more commonly?

I’ve tried searching both google and stackoverflow for “ADL termination” and “argument dependent lookup termination” and I haven’t found anything relevant.

I just don’t understand the word “termination” in the way you’re using it so I don’t understand what that statement means. Help?

#59

Thanks for the explanation. When I’m asked to review code, I treat the pull request as a final product and don’t really care how it got there.

What I noticed about this change was that it’s gone from two arguments to one arguments in this constructor.

Just to be more specific with terminology, there’s no generated code with respect to templating. That said, the idea holds. I don’t expect it to be faster or slower, but we’re very aware of differences in behavior and bugs in the compiler that are completely opaque. When there’s a change like this, I’m just looking for a sanity check. I just want to know that if something this core changes, that we have taken a quick look to make sure it doesn’t adversely affect everything that touches it, even if it’s not related to complex numbers.

I don’t know how to check for that except empirically. We know there are weird compiler behaviors that are well out of our control. And I don’t expect a full sweep over all possible compilers / configurations. Just a quick check to see that something like this doesn’t tank performance.

#60

Those would be std::vector<double> for both.

That’s not the question I was asking.

The question I’m asking is… do you ever want gradients with respect to both real and imaginary components?

#61

I have now. Did you do this because of issues with template metaprogramming or did you design it this way? It looks like we have versions of some of these template metaprograms already in existence. And might be easier to maintain and implement this if we stuck to the existing paradigm, unless necessary.

#63

Sorry – my fault for asking the question backwards!

Do you ever want gradients of both the real and imaginary components (with respect to whatever other variables out there)? Or do you ever only care about just gradients of the real component? Or just the imaginary component? But never both.

#67

@ChrisChiasson, what’s the deal with withdrawn posts? Any reason they weren’t just left here?

#69

I’ll do that when I review it.

Correct. We only need ordinary derivatives. So I think all we need is this:

\frac{d}{du}(x + y i) = \frac{d}{du}x + \left(\frac{d}{du} y\right) i

The naming will all be determined by the standard if this is just specializing std::complex for var.

I’ll have to do this if I’m reviewing it. I don’t know how the zeroing thing works yet, either.

Specialization of standard library templates are allowed. You can this in action already in the math lib with our specialization of std::numeric_limits<var>.

That’s a good point. I haven’t looked at the code yet. I wanted to understand what it was trying to do before diving in.

#70

We’d rather get the improvements in as long as we understand them. I don’t think anyone’s particularly possessive about any of the code or the designs that are in there now. Most of it’s been through many hands. Many of the things in the code are only there because we couldn’t think of a better way to do things or haven’t had time to do things like improve for C++11 or make known algorithmic improvements.

The problem with changing a lot of code at once is that it gets harder to review. I think it’s easier to review two changes of size N than one change of size 2N. I understand why this one’s so big, though. The scope and the complex (literally!) math and the fact that this is getting in on the very low-level infrastructure at the heart of Stan makes us want to proceed cautiously before just merging it.

#71

That comment’s out of date.

Normally, it won’t get called.

But in some instances, a vari will have member variables whose destructors need to be called for cleanup. They’re put on a separate stack of vari whose constructors get called — var_alloc_stack_ in autodiffstackstorage.hpp.

I tried to get rid of most of them, but there are some lingering cases. Those will call the destructor ~vari() on their way to cleaning up those classes.

We added a separate stack component to allow member variables that needed to have destructors called. So if you’re using one of those functions, it will call the destructor.

#72