Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Eigen double to matrix var assignment #3049

Merged
merged 11 commits into from
Jul 19, 2021

Conversation

SteveBronder
Copy link
Collaborator

Submission Checklist

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

Summary

This fixes the assign() errors from stan-dev/stanc3#898 for doing var<Matrix> = Eigen::Matrix<double>. Combining this with stan-dev/math#2535 removed the errors on the sample of test models I tried out in the stanc test suite that were previously failing.

Explicit paths had to be added in the callbacks for assignment using index_uni and index_multi. For all the other functions I wrote a assign_impl() function that is just A = B for all cases besides var<Matrix> = Eigen::Matrix<double>. For that case we do a procedure to update the var values and then in a reverse pass callback put back the old values and reset the adjoints to zero.

Intended Effect

Allow assignment from Eigen::Matrix<double> to var<Matrix> in the stanc generated C++.

How to Verify

All the old tests for var<Matrix> assignment were pulled out and made into functions that test assignment from both a var<Eigen::Matrix<double>> and Eigen::Matrix<double>. These can be run with

./runTests.py ./src/test/unit/model/indexing/assign_varmat_test.cpp

Side Effects

None

Documentation

Docs added for new assign_impl() function.

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): Steve Bronder

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@SteveBronder SteveBronder requested a review from t4c1 July 14, 2021 18:24
@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.17 3.08 1.03 2.81% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.99 -0.51% slower
eight_schools/eight_schools.stan 0.11 0.12 0.98 -2.4% slower
gp_regr/gp_regr.stan 0.16 0.16 0.99 -0.79% slower
irt_2pl/irt_2pl.stan 5.91 5.94 0.99 -0.57% slower
performance.compilation 89.05 86.84 1.03 2.48% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.57 9.06 0.95 -5.73% slower
pkpd/one_comp_mm_elim_abs.stan 30.02 29.47 1.02 1.82% faster
sir/sir.stan 130.55 128.36 1.02 1.68% faster
gp_regr/gen_gp_data.stan 0.03 0.03 1.0 -0.21% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 2.99 3.02 0.99 -1.06% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.41 0.4 1.04 4.14% faster
arK/arK.stan 2.55 2.53 1.01 0.86% faster
arma/arma.stan 0.65 0.65 1.0 -0.15% slower
garch/garch.stan 0.64 0.64 1.0 -0.02% slower
Mean result: 1.00208451384

Jenkins Console Log
Blue Ocean
Commit hash: e0f289e


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

src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign.hpp Outdated Show resolved Hide resolved
src/test/unit/model/indexing/util.hpp Show resolved Hide resolved
@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.06 3.08 0.99 -0.55% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.96 -3.7% slower
eight_schools/eight_schools.stan 0.11 0.11 0.99 -0.57% slower
gp_regr/gp_regr.stan 0.16 0.16 0.98 -1.83% slower
irt_2pl/irt_2pl.stan 5.87 5.93 0.99 -1.06% slower
performance.compilation 87.75 86.61 1.01 1.3% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.64 8.59 1.01 0.67% faster
pkpd/one_comp_mm_elim_abs.stan 29.1 30.16 0.96 -3.64% slower
sir/sir.stan 128.25 132.64 0.97 -3.42% slower
gp_regr/gen_gp_data.stan 0.03 0.03 1.0 -0.44% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 2.99 2.99 1.0 -0.14% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.38 0.41 0.94 -6.09% slower
arK/arK.stan 2.55 2.54 1.0 0.26% faster
arma/arma.stan 0.65 0.65 1.0 0.25% faster
garch/garch.stan 0.64 0.64 1.0 0.15% faster
Mean result: 0.988003723293

Jenkins Console Log
Blue Ocean
Commit hash: 247940a


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

src/stan/model/indexing/assign.hpp Outdated Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Show resolved Hide resolved
src/stan/model/indexing/assign_varmat.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@t4c1 t4c1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few more

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.07 3.07 1.0 -0.12% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.97 -2.98% slower
eight_schools/eight_schools.stan 0.11 0.12 0.97 -3.58% slower
gp_regr/gp_regr.stan 0.16 0.16 1.0 -0.48% slower
irt_2pl/irt_2pl.stan 5.88 5.94 0.99 -1.02% slower
performance.compilation 89.31 86.71 1.03 2.9% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.56 8.56 1.0 0.07% faster
pkpd/one_comp_mm_elim_abs.stan 29.42 28.91 1.02 1.76% faster
sir/sir.stan 128.75 126.78 1.02 1.53% faster
gp_regr/gen_gp_data.stan 0.03 0.03 1.01 0.69% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.01 3.05 0.99 -1.47% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.39 0.42 0.92 -8.92% slower
arK/arK.stan 2.55 2.56 0.99 -0.54% slower
arma/arma.stan 0.65 0.65 1.0 -0.49% slower
garch/garch.stan 0.65 0.64 1.02 1.92% faster
Mean result: 0.99361869631

Jenkins Console Log
Blue Ocean
Commit hash: 7cf50a2


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

@SteveBronder
Copy link
Collaborator Author

Shoot it won't let me click the merge button, can someone click it?

@rok-cesnovar rok-cesnovar merged commit 35fd563 into develop Jul 19, 2021
@rok-cesnovar rok-cesnovar deleted the fix/matrix-var-eigen-double-assign branch July 19, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants