Implemented reverse mode for symmetric eigendecomposition #1878

IvanYashchuk · 2020-05-13T07:46:34Z

Summary

This PR implements reverse mode differentiation for stan::math::eigenvectors_sym and stan::math::eigenvalues_sym.

Tests

There are already tests for AD with eigenvalues_sym and eigenvectors_sym.

I have added a test that checks the derivative of sum(log(eigenvalues_sym(A))). The derivative should be equal to the inverse of A. This test fails on the current develop branch. See #1803.

Side Effects

Currently, derivatives of eigenvalues_sym return lower triangular matrix.
I think that the correct derivative matrix should be symmetric and not lower triangular. This also affects any other function involved in the computation and the calculated autodiff derivative is not correct anymore. See #1803.

Release notes

Reverse mode differentiation of stan::math::eigenvectors_sym and stan::math::eigenvalues_sym now returns symmetric matrix (previously lower triangular).

Checklist

Math issues Functions in prim/mat/fun that could use custom reverse mode autodiff implementations #989, Wrong derivatives for cholesky and symmetric eigen decomposition #1803
Copyright holder: Ivan Yashchuk
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)

…vanYashchuk/math into feature/issue-989-rev-mat-eig2

bob-carpenter · 2020-05-13T15:07:46Z

Thanks! The code for this looks beautiful and is well-documented and references. I don't quite feel up to reviewing the linear algebra here or the implications for matrix autodiff. I'm totally happy to sign off on the code, especially as we have generic autodiff tests in place already.

stan-buildbot · 2020-05-13T17:05:26Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	5.07	5.09	1.0	-0.24% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.0	-0.3% slower
eight_schools/eight_schools.stan	0.09	0.09	0.98	-1.82% slower
gp_regr/gp_regr.stan	0.22	0.23	0.99	-1.09% slower
irt_2pl/irt_2pl.stan	6.56	6.57	1.0	-0.16% slower
performance.compilation	85.32	86.46	0.99	-1.33% slower
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.71	7.69	1.0	0.18% faster
pkpd/one_comp_mm_elim_abs.stan	20.37	20.62	0.99	-1.26% slower
sir/sir.stan	95.64	95.12	1.01	0.55% faster
gp_regr/gen_gp_data.stan	0.05	0.05	1.0	-0.31% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.01	3.06	0.98	-1.53% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.33	0.32	1.02	2.23% faster
arK/arK.stan	1.84	1.82	1.01	0.61% faster
arma/arma.stan	0.83	0.83	1.0	0.27% faster
garch/garch.stan	0.56	0.57	0.99	-0.5% slower
Mean result: 0.996973205053

Jenkins Console Log
Blue Ocean
Commit hash: ac9ee2e

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

bbbales2

Thanks for doing this! Looks good, just a couple simplifications. For the eigenvalues function, you don't need to make eigenvector varis (there won't be any vars created for these eigenvectors). Same for the eigenvectors function.

A little bit of math disappears from the chains as well (the adjoints of the eigenvectors for the eigenvalues function will be zero).

stan/math/rev/fun/eigenvalues_sym.hpp

stan/math/rev/fun/eigenvectors_sym.hpp

…s/RELEASE_600/final)

IvanYashchuk · 2020-05-19T15:42:25Z

@bbbales2, thank you for the review! I think I've addressed the comments.

bbbales2 · 2020-05-19T15:48:38Z

Thanks! Yeah the code looks great. I was just having a look at the original issue: #1803

I'm not convinced we want to do the full matrix of derivatives, if that makes sense. Can you take a look at what I posted over there? I kinda think the lower triangle is the way to go, as weird as that may seem.

bbbales2 · 2020-05-19T15:49:15Z

I kinda think

And by this argue back and tell me I'm wrong if I'm wrong I'm just trying to square it all in my head so I know what to do going forward.

stan-buildbot · 2020-05-19T17:49:57Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.98	4.02	0.99	-1.03% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.95	-5.34% slower
eight_schools/eight_schools.stan	0.09	0.09	0.96	-4.59% slower
gp_regr/gp_regr.stan	0.19	0.19	1.0	0.24% faster
irt_2pl/irt_2pl.stan	6.56	6.56	1.0	-0.02% slower
performance.compilation	86.73	86.53	1.0	0.22% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.73	7.72	1.0	0.1% faster
pkpd/one_comp_mm_elim_abs.stan	20.69	21.55	0.96	-4.15% slower
sir/sir.stan	92.32	92.41	1.0	-0.09% slower
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	-0.17% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.04	3.08	0.99	-1.25% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.32	0.33	0.97	-3.34% slower
arK/arK.stan	2.42	2.45	0.99	-1.04% slower
arma/arma.stan	0.61	0.61	1.0	-0.28% slower
garch/garch.stan	0.52	0.52	1.0	-0.1% slower
Mean result: 0.986627662748

Jenkins Console Log
Blue Ocean
Commit hash: b2ab104

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

stan-buildbot · 2020-05-19T22:55:26Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.04	4.05	1.0	-0.28% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.0	0.2% faster
eight_schools/eight_schools.stan	0.09	0.09	0.96	-4.45% slower
gp_regr/gp_regr.stan	0.19	0.19	1.0	0.49% faster
irt_2pl/irt_2pl.stan	6.59	6.58	1.0	0.12% faster
performance.compilation	86.97	86.23	1.01	0.84% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.77	7.7	1.01	0.95% faster
pkpd/one_comp_mm_elim_abs.stan	21.68	20.37	1.06	6.03% faster
sir/sir.stan	92.77	92.52	1.0	0.27% faster
gp_regr/gen_gp_data.stan	0.04	0.04	0.99	-0.66% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.2	3.04	1.05	5.01% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.31	0.99	-1.48% slower
arK/arK.stan	2.43	2.42	1.0	0.25% faster
arma/arma.stan	0.61	0.61	1.01	0.86% faster
garch/garch.stan	0.52	0.52	1.0	-0.13% slower
Mean result: 1.00595463618

Jenkins Console Log
Blue Ocean
Commit hash: b2ab104

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

bbbales2 · 2020-05-22T17:53:40Z

I'm gonna just merge this once the tests pass. I didn't notice they were hung up.

stan-buildbot · 2020-05-22T23:50:19Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.13	4.01	1.03	2.74% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-1.92% slower
eight_schools/eight_schools.stan	0.09	0.09	1.0	-0.4% slower
gp_regr/gp_regr.stan	0.2	0.19	1.05	4.88% faster
irt_2pl/irt_2pl.stan	6.57	6.56	1.0	0.08% faster
performance.compilation	85.34	86.06	0.99	-0.84% slower
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.79	7.71	1.01	1.06% faster
pkpd/one_comp_mm_elim_abs.stan	20.82	20.83	1.0	-0.02% slower
sir/sir.stan	92.51	94.49	0.98	-2.14% slower
gp_regr/gen_gp_data.stan	0.04	0.04	1.01	0.95% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.03	3.08	0.98	-1.8% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.32	0.31	1.04	3.77% faster
arK/arK.stan	2.41	2.42	1.0	-0.35% slower
arma/arma.stan	0.6	0.61	0.98	-1.72% slower
garch/garch.stan	0.52	0.52	1.01	1.15% faster
Mean result: 1.00407535416

Jenkins Console Log
Blue Ocean
Commit hash: a2d62b7

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

bbbales2 · 2020-05-23T16:05:06Z

@rok-cesnovar this is good to go. I made a change to how the tests are organized (just moved code from mix to rev in tests here), so I'm just leaving this to you to check I didn't do anything obviously silly and merge.

rok-cesnovar · 2020-05-23T17:11:28Z

That change look ok to me. The test tests rev so that is where it should be. Merging.

IvanYashchuk and others added 13 commits March 28, 2020 23:06

Started work on eig

5d8ee53

Updated headers

4be76ca

Fixed mistake with cherry-picking commit

928dda9

Removed debug prints

5806699

Merge branch 'develop' into feature/issue-989-rev-mat-eig2

a8e8055

Added check_nonzero_size

d47b2d9

Added test for stan-dev/issues/1803

bb9a1fa

cpplint

f2f29c2

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

7bc3498

…4.1 (tags/RELEASE_600/final)

Added missing header

8535715

Merge branch 'feature/issue-989-rev-mat-eig2' of https://github.com/I…

df1271e

…vanYashchuk/math into feature/issue-989-rev-mat-eig2

Apply checks on matrix_d value of input

a9735d1

Added value_of_rec header

ac9ee2e

bbbales2 self-assigned this May 18, 2020

bbbales2 requested changes May 18, 2020

View reviewed changes

IvanYashchuk and others added 5 commits May 19, 2020 14:56

Use alloc_array<T> instead of alloc

3ddbf58

Deleted comment

dc1a39e

Removed unnecessary computations

a6c7981

Merge commit '19f5f25bf9de1d267c92522d68eccfd0d73e12d0' into HEAD

e958b73

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2 (tag…

b2ab104

…s/RELEASE_600/final)

IvanYashchuk mentioned this pull request May 19, 2020

Wrong derivatives for cholesky and symmetric eigen decomposition #1803

Open

Moved new symmetry tests from mix to rev (stan-dev#1803)

a2d62b7

bbbales2 approved these changes May 23, 2020

View reviewed changes

rok-cesnovar merged commit d584931 into stan-dev:develop May 23, 2020

IvanYashchuk deleted the feature/issue-989-rev-mat-eig2 branch May 23, 2020 18:25

rok-cesnovar mentioned this pull request Jul 20, 2020

Release of Stan Math 3.3 #1974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented reverse mode for symmetric eigendecomposition #1878

Implemented reverse mode for symmetric eigendecomposition #1878

IvanYashchuk commented May 13, 2020

bob-carpenter commented May 13, 2020

stan-buildbot commented May 13, 2020

bbbales2 left a comment

IvanYashchuk commented May 19, 2020

bbbales2 commented May 19, 2020

bbbales2 commented May 19, 2020

stan-buildbot commented May 19, 2020

stan-buildbot commented May 19, 2020

bbbales2 commented May 22, 2020

stan-buildbot commented May 22, 2020

bbbales2 commented May 23, 2020

rok-cesnovar commented May 23, 2020

Implemented reverse mode for symmetric eigendecomposition #1878

Implemented reverse mode for symmetric eigendecomposition #1878

Conversation

IvanYashchuk commented May 13, 2020

Summary

Tests

Side Effects

Release notes

Checklist

bob-carpenter commented May 13, 2020

stan-buildbot commented May 13, 2020

bbbales2 left a comment

Choose a reason for hiding this comment

IvanYashchuk commented May 19, 2020

bbbales2 commented May 19, 2020

bbbales2 commented May 19, 2020

stan-buildbot commented May 19, 2020

stan-buildbot commented May 19, 2020

bbbales2 commented May 22, 2020

stan-buildbot commented May 22, 2020

bbbales2 commented May 23, 2020

rok-cesnovar commented May 23, 2020