Issue 123 #643

bgoodri · 2017-10-08T18:40:10Z

Submission Checklist

Run unit tests: ./runTests.py test/unit
Run cpplint: make cpplint
Declare copyright holder and open-source license: see below

Summary:

Define some kinda safe var constructors when the input is std::complex. Eigen uses complex numbers internally for some calculations, so we want to find out if we are accidentally using them.

Intended Effect:

For the case where the input is std::complex but the imaginary part is zero, create a var from the real part. For the case where the input is std::complex<var>, just fail.

How to Verify:

There are some tests for the non-failing case. I don't know how to test the failing cases because they call assert.

Side Effects:

Fixes #123 sort of.

R may complain, but at least on Linux the compiler seems to get rid of these constructors when they are not used.

Documentation:

None

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): Trustees of Columbia University

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/)

seantalts · 2017-10-13T00:30:29Z

It seems like we never use cassert's assert() in the math library - do we normally use something else for this kind of error handling? It seems like maybe this could happen at runtime for a user if we allowed complex parameters - should we throw an exception with an error message?

bgoodri · 2017-10-13T00:40:51Z

There is basically no danger of a complex number accidentally being created by the user. The only danger is that Eigen, Boost, CVODES, etc. use complex numbers for internal calculations, which would mess up the autodiff at a minimum. Eigen is known to do this, so I wanted to catch it in the tests.

…

On Thu, Oct 12, 2017 at 8:30 PM, seantalts ***@***.***> wrote: It seems like we never use cassert's assert() in the math library - do we normally use something else for this kind of error handling? It seems like maybe this could happen at runtime for a user if we allowed complex parameters - should we throw an exception with an error message? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#643 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrqsZgiffCSC5UMG5ReAT3qDS7WtrTks5srq8lgaJpZM4Pxzeb> .

bob-carpenter

Complex arguments should be passed by constant reference.

The asserts should be exceptions now that the construtor is in the math library. That will make sure we don't hit some new execution branch of Eigen at runtime and crash R or Python and will also make it safe for clients of the math library.

Otherwise, the functionality and doc looks great.

bob-carpenter · 2017-10-14T17:10:17Z

stan/math/rev/core/var.hpp

+       *
+       * @param x Value of the variable.
+       */
+      explicit var(std::complex<double> x) {


Instances of complex should be passed by constant reference, var(const std::complex<double>& x).

This change results in a compiler error

error: cannot bind non-const lvalue reference of type ‘std::complex<long double>&’ to an rvalue of type ‘std::complex<long double>’

Nevermind. I forgot the const.

bob-carpenter · 2017-10-14T17:14:49Z

stan/math/rev/core/var.hpp

+       * @param x Value of the variable.
+       */
+      explicit var(std::complex<double> x) {
+        assert(imag(x) == 0);


This should be an exception. Although users of the Stan language won't explicitly call it, it's going to be a constructor in the math library.

I don't know how much printing for complex is built in, but maybe

if (imag(x) == 0) { std::stringstream ss; ss << "Imaginary part of std::complex used to construct var must be zero." << " Found argument = " << x << std::endl; std::string msg = ss.str(); throw std::invalid_argument(msg); }

bob-carpenter · 2017-10-14T17:16:50Z

stan/math/rev/core/var.hpp

+       * @param x Value of the variable.
+       */
+      explicit var(std::complex<float> x) {
+        assert(imag(x) == 0);


same comment as before on pass-by-reference and exception

bob-carpenter · 2017-10-14T17:17:03Z

stan/math/rev/core/var.hpp

+       * @param x Value of the variable.
+       */
+      explicit var(std::complex<long double> x) {
+        assert(imag(x) == 0);


same comment as previous on exception and pass by reference

bob-carpenter · 2017-10-14T17:20:15Z

stan/math/rev/core/var.hpp

+       * @param x Value of the variable.
+       */
+      explicit var(std::complex<var> x) {
+        assert(false);


I don't think you need this constructor at all. If you try to call var(complex<var>(2,0)) it won't have a constructor to match, so the compiler will catch it. If complex<T> can be cast to T (I'm not sure, but I'd be surprised), then you want to declare this constructor but not define it. That will prevent it from being instantiated at compile time.

explicit var(const std::complex<var>& x);

This does not prevent the segfault in #123 but neither does filling in the body to only throw an exception. I don't know what to do but am pushing anyway.

The segfault in #123 is from constructing std::complex<var>(var(1)) not var(std::complex(1,0)). Are they related?

I don't think we can specialize the complex<T> constructor to complex<var> to fail.

bob-carpenter · 2017-10-14T17:22:36Z

stan/math/rev/core/var.hpp

@@ -7,6 +7,9 @@
 #include <boost/math/tools/config.hpp>
 #include <ostream>
 #include <vector>
+#include <complex>
+#include <cassert>
+#include <limits>


I didn't see anywhere <limits> gets used.

You won't need <cassert> after changing the asserts to exceptions.

still does not prevent segfault

syclik

Just noticed that it's missing stdexcept

syclik · 2017-10-20T13:42:25Z

stan/math/rev/core/var.hpp

@@ -7,6 +7,8 @@
 #include <boost/math/tools/config.hpp>
 #include <ostream>
 #include <vector>
+#include <complex>
+#include <string>


Need to include stdexcept.

syclik · 2017-10-20T13:45:23Z

@bob-carpenter, Ben has walked through all the changes. Want to approve?

I noticed that there's an exception being thrown in the constructor -- we good with that? I don't see us using this yet, but I just want to make sure that's the right pattern to be using for errors.

bob-carpenter · 2017-10-21T14:15:37Z

@syclik I approved it and will let you take over the final review.

I don't see a problem with throwing exceptions in constructors. Lots of our constructors already do that implicitly for vari by calling other functions that can throw exceptions.

Is there something I should be worried about here?

syclik · 2017-10-21T16:06:54Z

Nope. I just wanted to make sure I hadn't considered something obvious with exceptions in constructors.

@bgoodri, add the include and we're good!

syclik

Thanks!

bob-carpenter · 2017-10-24T20:37:42Z

I thought this and many of these others were merged. This is a direct conflict with the cpplint fix for tests and it's going to hit all 15 or so outstanding math pull requests as they changed lots of lines.

seantalts · 2017-10-24T20:52:50Z

@bob-carpenter Git is a lot better at merging than Subversion was - the only outstanding pull request that has a conflict due to the test folder reformatting is mine for removing row vector distribution tests. There are 2 others with unrelated conflicts. I don't think we want to try to coordinate PR merging to try to avoid conflicts - this has been a big waste of time in my professional life whenever it was attempted.

seantalts · 2017-10-24T21:05:30Z

Woops, my bad - #622 and #533 also have conflicts from the test formatting. Still, they should be easy enough to fix and one of those had been sitting there for a while.

seantalts · 2017-11-06T23:38:31Z

Jenkins, retest this please.

…_501/final)

seantalts · 2018-02-08T17:48:50Z

Should we merge this?

bgoodri · 2018-02-08T17:55:56Z

I think it does not hurt anything, but it does not help all that much.

seantalts · 2018-02-08T20:18:21Z

Okay, I'll merge it now that the tests are passing. No sense in keeping it around in limbo.

bob-carpenter · 2018-03-05T22:05:34Z

I deleted my last comment after talking to @seantalts and figuring out what's really going on.

@ChrisChiasson is right in that this PR as merged doesn't directly address issue #123. I think in this case, the PR is as intended, whereas the issue was poorly specified.

The one question I have about it is that it doesn't just throw, but actually lets you construct an autodiff variable from a complex number with zero imaginary component. That was not the feature request.

Is this really the right fix?

bgoodri added 2 commits October 8, 2017 13:58

add var dummy constructors in the complex case

60e238e

add and test a few more cases

f90037a

bob-carpenter requested changes Oct 14, 2017

View reviewed changes

bgoodri added 3 commits October 14, 2017 14:06

make suggested changes

8676635

still does not prevent segfault

remove blank line

fceed9b

eliminate constructor

5d99ffb

syclik requested changes Oct 20, 2017

View reviewed changes

bob-carpenter approved these changes Oct 21, 2017

View reviewed changes

add exception header

30d608d

syclik approved these changes Oct 22, 2017

View reviewed changes

Merge branch 'develop' into issue-123

9d1a8d0

yashikno and others added 2 commits January 31, 2018 09:13

Merge commit 'bdc750117a4ea9256061a2f26fd974bd2b63e624' into HEAD

ff404f7

[Jenkins] auto-formatting by clang-format version 5.0.1 (tags/RELEASE…

8200065

…_501/final)

seantalts merged commit 5ea9efa into develop Feb 8, 2018

seantalts deleted the issue-123 branch February 8, 2018 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 123 #643

Issue 123 #643

bgoodri commented Oct 8, 2017 •

edited by syclik

Loading

seantalts commented Oct 13, 2017

bgoodri commented Oct 13, 2017 via email

bob-carpenter left a comment

bob-carpenter Oct 14, 2017

bgoodri Oct 14, 2017

bgoodri Oct 14, 2017

bob-carpenter Oct 14, 2017

bob-carpenter Oct 14, 2017

bob-carpenter Oct 14, 2017

bob-carpenter Oct 14, 2017

bgoodri Oct 14, 2017

bob-carpenter Oct 14, 2017 •

edited

Loading

bob-carpenter Oct 14, 2017

syclik left a comment

syclik Oct 20, 2017

syclik commented Oct 20, 2017

bob-carpenter commented Oct 21, 2017

syclik commented Oct 21, 2017

syclik left a comment

bob-carpenter commented Oct 24, 2017

seantalts commented Oct 24, 2017

seantalts commented Oct 24, 2017

seantalts commented Nov 6, 2017

seantalts commented Feb 8, 2018

bgoodri commented Feb 8, 2018

seantalts commented Feb 8, 2018

bob-carpenter commented Mar 5, 2018 •

edited

Loading

Issue 123 #643

Issue 123 #643

Conversation

bgoodri commented Oct 8, 2017 • edited by syclik Loading

Submission Checklist

Summary:

Intended Effect:

How to Verify:

Side Effects:

Documentation:

Copyright and Licensing

seantalts commented Oct 13, 2017

bgoodri commented Oct 13, 2017 via email

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bob-carpenter Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syclik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syclik commented Oct 20, 2017

bob-carpenter commented Oct 21, 2017

syclik commented Oct 21, 2017

syclik left a comment

Choose a reason for hiding this comment

bob-carpenter commented Oct 24, 2017

seantalts commented Oct 24, 2017

seantalts commented Oct 24, 2017

seantalts commented Nov 6, 2017

seantalts commented Feb 8, 2018

bgoodri commented Feb 8, 2018

seantalts commented Feb 8, 2018

bob-carpenter commented Mar 5, 2018 • edited Loading

bgoodri commented Oct 8, 2017 •

edited by syclik

Loading

bob-carpenter Oct 14, 2017 •

edited

Loading

bob-carpenter commented Mar 5, 2018 •

edited

Loading