Feature/1886 /test utility cleanup - part 1 #1895

rok-cesnovar · 2020-05-19T12:24:23Z

Summary

Addresses the first part of #1886 by defining the following macros:

EXPECT_MATRIX_EQ
EXPECT_MATRIX_FLOAT_EQ
EXPECT_MATRIX_NEAR
EXPECT_STD_VECTOR_FLOAT_EQ
EXPECT_TYPED_MATRIX_EQ
EXPECT_TYPED_MATRIX_NEAR

These are all defined in test/unit/util.hpp. This PR removes all the duplicate definitions of these macros in various test files and removes the
expect_std_vector_eq and expect_matrix_eq functions in favor of the macros.

This also fixes an issue with returning a non-const ref in eigen_plugin.h.

Tests

/

Side Effects

/

Release notes

Unified EXPECT_MATRIX_* and EXPECT_STD_VECTOR_* macros in the Math unit tests.

Checklist

Math issue Organize and cleanup the use of utils in test/ #1886
Copyright holder: Rok Češnovar

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)

…s/RELEASE_600/final)

This reverts commit 1ab2f2f. # Conflicts: # test/unit/util.hpp

…4.1 (tags/RELEASE_600/final)

…s/RELEASE_600/final)

rok-cesnovar · 2020-05-21T12:49:13Z

This is ready for review.

syclik · 2020-05-21T13:56:30Z

Thank you for putting this together! Some things that should be part of this PR:

some basic code doc for the macros (inside util.hpp)
some documentation for developers so they know that these macros are part of Stan Math's code and not Google Test. This can be in the wiki or doxygen or even some readme file. As long as it's there, I'd be happy. And... it can just be something like a list of macros added with what the arguments expect... or something simple like that. Having just something is better than nothing here.

Otherwise, this is great. I'll give it a code-review soon.

stan-buildbot · 2020-05-21T16:11:27Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.0	3.93	1.02	1.82% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.0	0.24% faster
eight_schools/eight_schools.stan	0.09	0.09	1.0	0.41% faster
gp_regr/gp_regr.stan	0.19	0.19	1.0	-0.4% slower
irt_2pl/irt_2pl.stan	6.56	6.56	1.0	0.01% faster
performance.compilation	85.52	84.42	1.01	1.29% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.72	7.73	1.0	-0.13% slower
pkpd/one_comp_mm_elim_abs.stan	20.53	21.1	0.97	-2.8% slower
sir/sir.stan	92.38	92.88	0.99	-0.54% slower
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	-0.09% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.03	3.04	1.0	-0.21% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.33	0.32	1.04	3.71% faster
arK/arK.stan	2.43	2.42	1.0	0.49% faster
arma/arma.stan	0.61	0.6	1.01	0.83% faster
garch/garch.stan	0.52	0.52	1.0	0.15% faster
Mean result: 1.00336856182

Jenkins Console Log
Blue Ocean
Commit hash: 98c4675

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-21T22:50:27Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.07	3.93	1.04	3.63% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.01% slower
eight_schools/eight_schools.stan	0.09	0.09	0.96	-4.62% slower
gp_regr/gp_regr.stan	0.19	0.19	1.02	1.99% faster
irt_2pl/irt_2pl.stan	6.56	6.56	1.0	-0.05% slower
performance.compilation	86.78	84.37	1.03	2.79% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.73	7.71	1.0	0.19% faster
pkpd/one_comp_mm_elim_abs.stan	20.98	21.37	0.98	-1.89% slower
sir/sir.stan	90.88	90.72	1.0	0.18% faster
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	-0.48% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.08	3.03	1.02	1.59% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.31	1.01	0.98% faster
arK/arK.stan	2.41	2.43	0.99	-0.84% slower
arma/arma.stan	0.61	0.61	1.0	0.27% faster
garch/garch.stan	0.51	0.52	0.98	-1.61% slower
Mean result: 1.00048908424

Jenkins Console Log
Blue Ocean
Commit hash: 98c4675

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

rok-cesnovar · 2020-05-22T07:07:58Z

I agree. Will add a section to the wiki and once that is ready will formally request your review here on the PR.

…util-cleanup-pt1

rok-cesnovar · 2020-05-26T08:04:21Z

@syclik I updated the wiki: https://github.com/stan-dev/math/wiki/Developer-Doc#writing-tests (the text assumes this PR gets merged) and also added the doxygen to the util file.

I am waiting for #1865 to get merged so we can also use the util file there.

Other than that this is ready for review.

…s/RELEASE_600/final)

…util-cleanup-pt1

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2020-05-26T21:19:14Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.99	4.0	1.0	-0.38% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.96	-3.75% slower
eight_schools/eight_schools.stan	0.09	0.09	1.03	2.55% faster
gp_regr/gp_regr.stan	0.19	0.19	0.98	-2.45% slower
irt_2pl/irt_2pl.stan	6.55	6.58	1.0	-0.45% slower
performance.compilation	84.62	83.67	1.01	1.12% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.7	7.71	1.0	-0.08% slower
pkpd/one_comp_mm_elim_abs.stan	20.57	21.1	0.97	-2.56% slower
sir/sir.stan	92.29	93.13	0.99	-0.92% slower
gp_regr/gen_gp_data.stan	0.04	0.04	0.99	-0.63% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.05	3.04	1.0	0.12% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.32	0.96	-4.09% slower
arK/arK.stan	2.42	2.42	1.0	-0.29% slower
arma/arma.stan	0.61	0.61	1.0	-0.14% slower
garch/garch.stan	0.52	0.52	0.99	-0.55% slower
Mean result: 0.99201062817

Jenkins Console Log
Blue Ocean
Commit hash: 87cf933

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-28T09:24:18Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.97	3.95	1.01	0.54% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-3.26% slower
eight_schools/eight_schools.stan	0.09	0.09	1.01	0.54% faster
gp_regr/gp_regr.stan	0.19	0.19	1.0	0.34% faster
irt_2pl/irt_2pl.stan	6.59	6.55	1.01	0.67% faster
performance.compilation	84.79	83.5	1.02	1.52% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.72	7.72	1.0	0.06% faster
pkpd/one_comp_mm_elim_abs.stan	21.26	21.46	0.99	-0.93% slower
sir/sir.stan	90.97	92.6	0.98	-1.8% slower
gp_regr/gen_gp_data.stan	0.04	0.04	0.96	-3.77% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.08	3.04	1.02	1.54% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.33	0.31	1.04	4.27% faster
arK/arK.stan	2.43	2.42	1.0	0.47% faster
arma/arma.stan	0.61	0.61	1.0	0.11% faster
garch/garch.stan	0.51	0.52	0.99	-0.62% slower
Mean result: 1.00014318832

Jenkins Console Log
Blue Ocean
Commit hash: 87cf933

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

rok-cesnovar · 2020-06-09T12:28:56Z

@syclik do you have time to review this PR this week? No problem if not, just so I know.

syclik

Thank you for the work! There’s just a question with the Eigen plugins header. There was a change that I didn’t know if it was supposed to be part of this PR.

Thanks for documenting the macros!!!!

syclik · 2020-06-09T12:32:07Z

stan/math/prim/eigen_plugins.h

@@ -53,14 +53,14 @@ struct val_Op{
  //Returns value from a vari*
  template<typename T = Scalar>
  EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE
-    std::enable_if_t<std::is_pointer<T>::value, reverse_return_t<T>>
+    std::enable_if_t<std::is_pointer<T>::value, const double&>


Were these changes intentionally part of this PR?

Yes, this was a bug. I mention it in the top-level comment: "This also fixes an issue with returning a non-const ref in eigen_plugin.h.".

Basically this should return a const ref, not a non-const.

rok-cesnovar · 2020-06-22T11:39:45Z

Bumping this. No real rush, but I would like to proceed with the rest of test cleanup PRs I have lined up. Once the cleanup is done we can do some optimization of the testing to make it a lot quicker with the same test coverage.

syclik

Thanks!

syclik · 2020-06-22T11:49:28Z

Sorry, I missed that you had replied; feel free to ping me directly if it looks like I missed a comment.

rok-cesnovar · 2020-06-22T11:50:11Z

No problem, I dont like to bug people to much :) Thanks!

Bugfix/develop after #1895

rok-cesnovar and others added 15 commits May 18, 2020 19:46

move EXPECT_MATRIX_NEAR

0f54b62

fix some EXPECT_MATRIX_NEAR calls

6c0323d

replace expect_matrix_eq with macro

88efd57

remove unused expect_matrix_eq

1a22845

replace header

d580190

fix eigen plugins to

77c8edf

fix util

d71ae92

eval the matrices

145fd59

replace expect_std_vector_eq

3fe7e94

add EXPECT_MATRIX_EQ macro

48bb2a6

fix macro

c041582

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

be8b47f

…4.1 (tags/RELEASE_600/final)

fix includes

9f10b21

fix

04ea752

fix test parenthesis

75989bf

rok-cesnovar changed the title ~~Feature/1886 test util cleanup pt1~~ Feature/1886 /test utility cleanup - part 1 May 19, 2020

rok-cesnovar and others added 12 commits May 20, 2020 15:03

fix test and expand macro

1ab2f2f

Merge commit '4a3cd07bd2a1bfc3b7ebeba9bd777b1494f9efcd' into HEAD

55863c1

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

4c3e485

…s/RELEASE_600/final)

replace auto in macros

0ff7959

Revert "fix test and expand macro"

8e667c9

This reverts commit 1ab2f2f. # Conflicts: # test/unit/util.hpp

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

8725bf0

…4.1 (tags/RELEASE_600/final)

fix macro

4102d9b

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

85e5825

…4.1 (tags/RELEASE_600/final)

fix for type

5b46ca2

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

e5fcee5

…s/RELEASE_600/final)

Merge branch 'develop' into feature/1886-test-util-cleanup-pt1

ea1a61d

fix add test

98c4675

rok-cesnovar added 2 commits May 26, 2020 07:42

Merge remote-tracking branch 'origin/develop' into feature/1886-test-…

f471171

…util-cleanup-pt1

add doxygen

c00064f

stan-buildbot and others added 3 commits May 26, 2020 04:30

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

525601e

…s/RELEASE_600/final)

Merge remote-tracking branch 'origin/develop' into feature/1886-test-…

b3903da

…util-cleanup-pt1

cleanup the remaining macros

c909a64

rok-cesnovar requested a review from syclik May 26, 2020 14:23

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

87cf933

…4.1 (tags/RELEASE_600/final)

syclik reviewed Jun 9, 2020

View reviewed changes

rok-cesnovar requested a review from syclik June 15, 2020 09:48

syclik approved these changes Jun 22, 2020

View reviewed changes

rok-cesnovar merged commit 2a7a857 into develop Jun 22, 2020

rok-cesnovar deleted the feature/1886-test-util-cleanup-pt1 branch June 22, 2020 11:50

rok-cesnovar mentioned this pull request Jun 22, 2020

Bugfix/develop after #1895 #1946

Merged

5 tasks

rok-cesnovar added a commit that referenced this pull request Jun 22, 2020

Merge pull request #1946 from stan-dev/bugfix/missing-file

de6e454

Bugfix/develop after #1895

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

Feature/1886 /test utility cleanup - part 1 #1895

Feature/1886 /test utility cleanup - part 1 #1895

rok-cesnovar commented May 19, 2020 •

edited

Loading

rok-cesnovar commented May 21, 2020

syclik commented May 21, 2020

stan-buildbot commented May 21, 2020

stan-buildbot commented May 21, 2020

rok-cesnovar commented May 22, 2020

rok-cesnovar commented May 26, 2020

stan-buildbot commented May 26, 2020

stan-buildbot commented May 28, 2020

rok-cesnovar commented Jun 9, 2020 •

edited

Loading

syclik left a comment

syclik Jun 9, 2020

rok-cesnovar Jun 9, 2020

rok-cesnovar commented Jun 22, 2020

syclik left a comment

syclik commented Jun 22, 2020

rok-cesnovar commented Jun 22, 2020

Feature/1886 /test utility cleanup - part 1 #1895

Feature/1886 /test utility cleanup - part 1 #1895

Conversation

rok-cesnovar commented May 19, 2020 • edited Loading

Summary

Tests

Side Effects

Release notes

Checklist

rok-cesnovar commented May 21, 2020

syclik commented May 21, 2020

stan-buildbot commented May 21, 2020

stan-buildbot commented May 21, 2020

rok-cesnovar commented May 22, 2020

rok-cesnovar commented May 26, 2020

stan-buildbot commented May 26, 2020

stan-buildbot commented May 28, 2020

rok-cesnovar commented Jun 9, 2020 • edited Loading

syclik left a comment

Choose a reason for hiding this comment

syclik Jun 9, 2020

Choose a reason for hiding this comment

rok-cesnovar Jun 9, 2020

Choose a reason for hiding this comment

rok-cesnovar commented Jun 22, 2020

syclik left a comment

Choose a reason for hiding this comment

syclik commented Jun 22, 2020

rok-cesnovar commented Jun 22, 2020

rok-cesnovar commented May 19, 2020 •

edited

Loading

rok-cesnovar commented Jun 9, 2020 •

edited

Loading