Complex numbers get_real(), get_imag(), to_complex #2554

nickdidio · 2021-08-04T16:08:08Z

Summary

Adding support for complex scalar functions get_real(), get_imag() and to_complex().
get_real() takes in a complex number and outputs its real component, get_imag() does the same but for the imaginary component.
to_complex() takes in either 0, 1, or 2 ints or doubles and returns a complex number with its real and imaginary component set to the inputs, and any unfilled components set to 0.

Tests

Test files are available in test/unit/math/mix/fun and are named get_real_test.hpp, get_imag_test.hpp and to_complex_test.hpp

Tests can be ran with:
./runTests.py test/unit/math/mix/fun/get_real_test.hpp
./runTests.py test/unit/math/mix/fun/get_imag_test.hpp
./runTests.py test/unit/math/mix/fun/to_complex_test.hpp

Side Effects

None

Release notes

Added complex scalar functions get_real(), get_imag(), and to_complex.

Checklist

Copyright holder: Nicholas DiDio

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

…4.1 (tags/RELEASE_600/final)

…nto complex-numbers

Steve's fix for apply_scalar_unary

SteveBronder

cool lgtm! @rok-cesnovar I know you wanted to think about the names here, are you and @bob-carpenter cool with get_real() and get_imag() imo a little wordy but they're fine

bob-carpenter

Lots of comments given this is your first PR. It shouldn't be hard to fix and I'm happy to sit down and work through it in more detail with you.

bob-carpenter · 2021-08-04T18:38:19Z

stan/math/prim/fun.hpp

@@ -270,7 +271,6 @@
 #include <stan/math/prim/fun/read_corr_matrix.hpp>
 #include <stan/math/prim/fun/read_cov_L.hpp>
 #include <stan/math/prim/fun/read_cov_matrix.hpp>
-#include <stan/math/prim/fun/real.hpp>


Don't remove real() and imag(). We won't use those functions by that name in Stan, but we need it to support programs written against the C++ standard library for our autodiff.

You can just have get_real() and get_imag() delegate to real() and imag() for implementation, but the generic form you have below is probably easier.

stan/math/prim/fun/to_complex.hpp

bob-carpenter · 2021-08-04T18:45:19Z

stan/math/prim/fun/to_complex.hpp

+ *
+ * Used for type casting from int to double for real component.
+ * Otherwise arrays of complex don't work with ints.
+ */


The following template shouldn't be necessary given the signature I suggested with promotion.

test/unit/math/mix/fun/to_complex_test.cpp

bob-carpenter · 2021-08-04T18:50:00Z

stan/math/prim/fun/to_complex.hpp

+}
+
+/**
+ * Return a complex type from a integer real part and an imaginary part.


The return is a complex value, not a complex type. And the arguments are a real part and imaginary part, but both are optional, which should be documented along with default values. For example,

@param[in] re real component (default = 0) @param[in] im imaginary component (default = 0)

You can see I also included the specification that both of these are input arguments (not things set by the program).

bob-carpenter · 2021-08-04T18:54:14Z

stan/math/prim/fun/get_real.hpp

@@ -14,8 +14,8 @@ namespace math {
 * @param[in] z argument


"argument" is not sufficient doc. This should say something like "value whose real component is extracted" or something like that.

bob-carpenter · 2021-08-04T18:55:42Z

stan/math/prim/fun/get_real.hpp

@@ -14,8 +14,8 @@ namespace math {
 * @param[in] z argument
 * @return real part of argument


[optional] it's nice to stay consistent in terminology. I'd suggest choosing "part" or "component" and sticking to it throughout the doc.

bob-carpenter · 2021-08-04T18:56:08Z

stan/math/prim/fun/to_complex.hpp

+ * Return a complex type from a real part and an imaginary part.
+ * Default values for both parts is 0.
+ *
+ * @tparam T type of real and


These don't need "and" (though that is how you'd write mathematics). And it should be "type of real component". The "type of real" would be double or float.

bob-carpenter · 2021-08-04T18:56:21Z

stan/math/prim/fun/to_complex.hpp

+ * @tparam T type of real and
+ * @param re real element
+ * @param im imaginary element
+ * @return Complex type


The return is a value, not a type.

bob-carpenter · 2021-08-04T18:57:48Z

stan/math/prim/fun/to_complex.hpp

+ * @param im imaginary element
+ * @return Complex type
+ *
+ * Used for type casting from int to double for real component.


Doc should be written actively in terms of what the function does, not how it's used (that can come later if it's not clear). So the doc for this would be the same as the doc or the previous example, only we shouldn't actually need this second function.

stan-buildbot · 2021-08-04T20:25:18Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	2.97	3.04	0.98	-2.48% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.73% slower
eight_schools/eight_schools.stan	0.1	0.11	0.92	-8.28% slower
gp_regr/gp_regr.stan	0.16	0.16	1.0	0.26% faster
irt_2pl/irt_2pl.stan	5.87	5.85	1.0	0.32% faster
performance.compilation	89.82	87.4	1.03	2.7% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.6	8.73	0.99	-1.48% slower
pkpd/one_comp_mm_elim_abs.stan	30.48	29.92	1.02	1.84% faster
sir/sir.stan	129.45	128.82	1.0	0.49% faster
gp_regr/gen_gp_data.stan	0.04	0.04	0.99	-0.77% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.98	3.16	0.95	-5.8% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.39	1.0	-0.14% slower
arK/arK.stan	1.86	1.85	1.01	0.59% faster
arma/arma.stan	0.83	0.84	0.99	-0.6% slower
garch/garch.stan	0.53	0.54	0.98	-1.62% slower
Mean result: 0.989040927377

Jenkins Console Log
Blue Ocean
Commit hash: 7b2f55a

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

…nto complex-numbers

…x-numbers" This reverts commit c401e78, reversing changes made to 7b2f55a. Revert apply_scalar_unary merge

…nto complex-numbers

…o/math into complex-numbers" This reverts commit 44fa4f9, reversing changes made to d2dc499.

…4.1 (tags/RELEASE_600/final)

…nickdidio/math into complex-numbers"" This reverts commit dc46d7e.

…nto complex-numbers

stan-buildbot · 2021-08-05T22:18:48Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.05	2.99	1.02	1.87% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.0	-0.07% slower
eight_schools/eight_schools.stan	0.11	0.11	0.98	-2.38% slower
gp_regr/gp_regr.stan	0.16	0.16	1.0	0.46% faster
irt_2pl/irt_2pl.stan	5.84	5.82	1.0	0.39% faster
performance.compilation	88.32	87.45	1.01	0.98% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.7	8.54	1.02	1.86% faster
pkpd/one_comp_mm_elim_abs.stan	31.07	31.4	0.99	-1.06% slower
sir/sir.stan	136.85	128.93	1.06	5.79% faster
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	-0.46% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.98	3.04	0.98	-1.89% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.39	0.98	-1.69% slower
arK/arK.stan	1.88	1.86	1.01	0.69% faster
arma/arma.stan	0.84	0.82	1.02	1.65% faster
garch/garch.stan	0.52	0.53	1.0	-0.27% slower
Mean result: 1.00430979176

Jenkins Console Log
Blue Ocean
Commit hash: 568f6d5

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

…nto complex-numbers

…4.1 (tags/RELEASE_600/final)

bob-carpenter

This is good to go when it passes tests.

nickdidio · 2021-08-09T15:25:01Z

So the tests for this are going to fail because it relies on Steve's apply-scalar-unary fix at #2549. I've tested both PR's together and it works. Not sure what to do about this, but it should work as soon as Steve's PR goes through.

…ex-numbers

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2021-08-11T01:35:35Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.0	3.02	0.99	-0.9% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.83% slower
eight_schools/eight_schools.stan	0.11	0.11	1.01	0.84% faster
gp_regr/gp_regr.stan	0.16	0.16	0.99	-0.85% slower
irt_2pl/irt_2pl.stan	5.93	5.88	1.01	0.87% faster
performance.compilation	88.36	87.36	1.01	1.13% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.6	8.68	0.99	-0.99% slower
pkpd/one_comp_mm_elim_abs.stan	30.53	29.67	1.03	2.82% faster
sir/sir.stan	128.63	128.39	1.0	0.19% faster
gp_regr/gen_gp_data.stan	0.03	0.04	0.98	-1.53% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.98	2.98	1.0	0.16% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.4	0.41	0.98	-1.78% slower
arK/arK.stan	1.88	1.9	0.99	-0.85% slower
arma/arma.stan	0.83	0.82	1.01	1.01% faster
garch/garch.stan	0.53	0.53	1.0	0.33% faster
Mean result: 0.998588819386

Jenkins Console Log
Blue Ocean
Commit hash: 899bc77

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

bob-carpenter

Thanks for fixing.

nickdidio and others added 11 commits June 24, 2021 10:53

to_complex, get_real, and get_imag functionality

293f224

Added to_complex() for no parameters

c26015d

Added comments

bf52b13

Added inline keyword

a670f91

Merge branch 'stan-dev:develop' into complex-numbers

a1844ca

Added test functions for get_real, get_imag, to_complex

f18711f

Merge branch 'stan-dev:develop' into complex-numbers

db24545

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

6289ec9

…4.1 (tags/RELEASE_600/final)

Merge branch 'complex-numbers' of https://github.com/nickdidio/math i…

336fbf6

…nto complex-numbers

Added newline to end of get_real_test.cpp and get_imag_test.cpp

7b2f55a

Merge branch 'feature/complex-apply-scalar-unary' into complex-numbers

c401e78

Steve's fix for apply_scalar_unary

SteveBronder previously approved these changes Aug 4, 2021

View reviewed changes

bob-carpenter requested changes Aug 4, 2021

View reviewed changes

nickdidio dismissed SteveBronder’s stale review via c401e78 August 5, 2021 16:28

nickdidio added 2 commits August 5, 2021 12:31

Merge branch 'complex-numbers' of https://github.com/nickdidio/math i…

d2dc499

…nto complex-numbers

Revert "Merge branch 'feature/complex-apply-scalar-unary' into comple…

c233d38

…x-numbers" This reverts commit c401e78, reversing changes made to 7b2f55a. Revert apply_scalar_unary merge

nickdidio marked this pull request as draft August 5, 2021 16:54

nickdidio and others added 6 commits August 5, 2021 14:15

Merge branch 'complex-numbers' of https://github.com/nickdidio/math i…

44fa4f9

…nto complex-numbers

Revert "Merge branch 'complex-numbers' of https://github.com/nickdidi…

dc46d7e

…o/math into complex-numbers" This reverts commit 44fa4f9, reversing changes made to d2dc499.

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

1edeb95

…4.1 (tags/RELEASE_600/final)

Revert "Revert "Merge branch 'complex-numbers' of https://github.com/…

da0160d

…nickdidio/math into complex-numbers"" This reverts commit dc46d7e.

Merge branch 'complex-numbers' of https://github.com/nickdidio/math i…

568f6d5

…nto complex-numbers

Fixed functions based on Bob's suggestions

766fe1d

nickdidio and others added 4 commits August 6, 2021 12:09

Cosine fix and complex_test fix

c1e9ebb

abs() fix

01eb5db

Merge branch 'complex-numbers' of https://github.com/nickdidio/math i…

fa3088b

…nto complex-numbers

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

01ee199

…4.1 (tags/RELEASE_600/final)

nickdidio marked this pull request as ready for review August 9, 2021 14:36

bob-carpenter previously approved these changes Aug 9, 2021

View reviewed changes

Merge branch 'develop' of https://github.com/stan-dev/math into compl…

0e21d72

…ex-numbers

nickdidio dismissed bob-carpenter’s stale review via 0e21d72 August 10, 2021 14:11

nickdidio and others added 6 commits August 10, 2021 10:32

Fixed test_ad

a8ccbec

Fixed some bugs

945c4d0

header fix for apply-scalar-unary

23e643f

More bug fixes

fc42112

Merge commit '62d9a8d03509e3e9cc8796b6e87b9a6fb375f2f6' into HEAD

f4da8d5

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

899bc77

…4.1 (tags/RELEASE_600/final)

bob-carpenter approved these changes Aug 11, 2021

View reviewed changes

bob-carpenter merged commit 02dc560 into stan-dev:develop Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complex numbers get_real(), get_imag(), to_complex #2554

Complex numbers get_real(), get_imag(), to_complex #2554

nickdidio commented Aug 4, 2021 •

edited

Loading

SteveBronder left a comment

bob-carpenter left a comment

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

bob-carpenter Aug 4, 2021

stan-buildbot commented Aug 4, 2021

stan-buildbot commented Aug 5, 2021

bob-carpenter left a comment

nickdidio commented Aug 9, 2021

stan-buildbot commented Aug 11, 2021

bob-carpenter left a comment

		@@ -14,8 +14,8 @@ namespace math {
		* @param[in] z argument
		* @return real part of argument

Complex numbers get_real(), get_imag(), to_complex #2554

Complex numbers get_real(), get_imag(), to_complex #2554

Conversation

nickdidio commented Aug 4, 2021 • edited Loading

Summary

Tests

Side Effects

Release notes

Checklist

SteveBronder left a comment

Choose a reason for hiding this comment

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

stan-buildbot commented Aug 4, 2021

stan-buildbot commented Aug 5, 2021

bob-carpenter left a comment

Choose a reason for hiding this comment

nickdidio commented Aug 9, 2021

stan-buildbot commented Aug 11, 2021

bob-carpenter left a comment

Choose a reason for hiding this comment

nickdidio commented Aug 4, 2021 •

edited

Loading