Cleanup requires with macros #1794

SteveBronder · 2020-03-23T18:50:21Z

Summary

This PR cleans the requires* meta programs to be auto generated using macros. This PR should be fully backwards compatible with the current method. More importantly, the documentation for these has been cleanup up such that it's much easier to go through and look for certain types of requires.

The macros are

STAN_ADD_REQUIRE_UNARY: For validating a single type satisfies a type trait.
STAN_ADD_REQUIRE_BINARY: For type traits that require comparing two types.
Both of these have a *_INNER macro that defines requires to check the scalar_type and value_type of arbitrary types. This is more useful for binary requires where we want to check that two matrices for instance have the same value_type. These are labeled as require_vt/st_* i.e. require_vt_same<T, S>

STAN_ADD_REQUIRE_CONTAINER: For defining requires like require_eigen_t<> and require_eigen_vt<>.

Tests

All the current tests should pass. One important thing to check is the documentation via make doxygen.

Side Effects

These macros will be floating in the background, but I think they are named such that it would be hard to accidentally hit them.

Release notes

Refactoring the require_* metaprograms to reduce code duplication and increase documentation.

Checklist

Math issue Breakup require_generics.hpp into multiple files #1739
Copyright holder: Steve Bronder

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

…tor to is_eigen and is_std_vector

…gs/RELEASE_500/final)

…stable/2017-11-14)

…gs/RELEASE_500/final)

SteveBronder · 2020-03-24T05:17:49Z

@serban-nicusor-toptal it looks like the windows machines are taking a bit? Fine with it taking a while if there are a bunch of PRs but just a heads up

serban-nicusor-toptal · 2020-03-24T08:24:15Z

Hey, one machine seems down. Going to get it up now to speed up the queue. Thanks for the notice!

stan-buildbot · 2020-03-24T10:01:59Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.82	4.91	0.98	-1.92% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.52% slower
eight_schools/eight_schools.stan	0.09	0.09	1.0	0.18% faster
gp_regr/gp_regr.stan	0.22	0.22	1.0	-0.2% slower
irt_2pl/irt_2pl.stan	6.47	6.44	1.0	0.36% faster
performance.compilation	89.03	86.65	1.03	2.67% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.53	7.54	1.0	-0.18% slower
pkpd/one_comp_mm_elim_abs.stan	20.77	20.9	0.99	-0.66% slower
sir/sir.stan	95.89	91.13	1.05	4.96% faster
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-1.48% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	3.11	0.95	-5.49% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.31	0.99	-0.65% slower
arK/arK.stan	1.74	1.74	1.0	0.22% faster
arma/arma.stan	0.66	0.66	0.99	-0.82% slower
garch/garch.stan	0.51	0.52	0.99	-1.43% slower
Mean result: 0.995887283487

Jenkins Console Log
Blue Ocean
Commit hash: ba5a07e

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

…stable/2017-11-14)

stan-buildbot · 2020-03-25T15:59:12Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.88	5.27	0.93	-7.93% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.92	-8.21% slower
eight_schools/eight_schools.stan	0.09	0.09	0.97	-3.05% slower
gp_regr/gp_regr.stan	0.22	0.22	0.98	-2.42% slower
irt_2pl/irt_2pl.stan	6.5	6.69	0.97	-2.94% slower
performance.compilation	89.03	87.4	1.02	1.82% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.53	7.71	0.98	-2.28% slower
pkpd/one_comp_mm_elim_abs.stan	20.65	20.78	0.99	-0.67% slower
sir/sir.stan	90.9	93.68	0.97	-3.06% slower
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-1.1% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	3.05	0.97	-3.42% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.33	0.95	-4.85% slower
arK/arK.stan	1.74	1.79	0.98	-2.55% slower
arma/arma.stan	0.66	0.68	0.97	-2.88% slower
garch/garch.stan	0.51	0.53	0.97	-3.24% slower
Mean result: 0.970288060688

Jenkins Console Log
Blue Ocean
Commit hash: dfdfdba

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 · 2020-03-25T20:37:11Z

@t4c1 would you have time to give this a lookover? It looks like a lot but it's mostly just moving stuff around and a good bit of docs. The only other change is that value_type and scalar_type's eigen and std vector specializations were moved to is_eigen and is_vector. I think this makes sense because you can't detect an eigen or std vector's inner types until you can detect whether you have an eigen type or std vector type.

That and I updated the code for the functions which were giving warnings on doxygen so that we could use the warnings are errors flag in doxygen. Now if anyone does not doc correctly it will error out

stan/math/prim/meta.hpp

README.md

doxygen/doxygen.cfg

t4c1 · 2020-03-26T10:44:56Z

stan/math/fwd/fun/sum.hpp

 * @return Sum of vector entries.
 */
 template <typename T>
-inline fvar<T> sum(const std::vector<fvar<T>>& m) {
-  if (m.size() == 0) {
+inline fvar<T> sum(const std::vector<fvar<T>>& v) {


Why did you rename all these variables? I don't think these have anything to do with requires. This PR is already large enough, so if you think these are needed, they should be put in a separate PR.

These are just to fix the doxygen warnings but I can undo them

To undo these I will need to turn off the doxygen option to promote warnings to errors that I added in this PR

If that is so, I am ok with leaving those in. I did not understand these are connected with docs.

Actually, what kind of warnings did renaming a few variables solve?

I see you already reverted those. Lets leave them for another PR.

t4c1 · 2020-03-26T10:51:23Z

stan/math/prim/meta.hpp

+/** \addtogroup type_trait
+ *  @{
+ */
+


is this just moved from stan/math.hpp?

Yeah I'll remove this

So this is now replaced with docs on each require(that are generated with macros)?

is this just moved from stan/math.hpp?

I meant the whole ~150 lines of docs.

stan/math/prim/meta/require_helpers.hpp

test/unit/math/prim/meta/require_generics_test.cpp

…ous doc fixes

…stable/2017-11-14)

…cleanup/require_base

…gs/RELEASE_500/final)

stan-buildbot · 2020-03-28T08:32:08Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.9	4.95	0.99	-0.9% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.92% slower
eight_schools/eight_schools.stan	0.09	0.09	1.0	0.31% faster
gp_regr/gp_regr.stan	0.22	0.22	0.98	-2.49% slower
irt_2pl/irt_2pl.stan	6.49	6.42	1.01	1.02% faster
performance.compilation	88.86	88.55	1.0	0.35% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.52	7.52	1.0	0.08% faster
pkpd/one_comp_mm_elim_abs.stan	21.85	20.9	1.05	4.36% faster
sir/sir.stan	94.18	90.76	1.04	3.63% faster
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-0.78% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	2.96	1.0	-0.18% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.31	0.99	-0.51% slower
arK/arK.stan	1.73	1.74	0.99	-0.68% slower
arma/arma.stan	0.66	0.66	0.99	-0.94% slower
garch/garch.stan	0.52	0.51	1.01	0.73% faster
Mean result: 1.00107751521

Jenkins Console Log
Blue Ocean
Commit hash: 70035f4

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

t4c1 · 2020-03-28T09:23:02Z

stan/math/prim/meta/require_generics.hpp

+STAN_ADD_REQUIRE_UNARY_INNER(floating_point, std::is_floating_point,
+                             require_stan_scalar_real);
+STAN_ADD_REQUIRE_UNARY(index, std::is_integral, require_stan_scalar_real);
+STAN_ADD_REQUIRE_UNARY_INNER(index, std::is_integral, require_stan_scalar_real);


I know it is not part of this PR, but why do we have require_index for is_integral?

tbh I put that in there whenever I was working on stan upstream stuff, I can change this

t4c1 · 2020-03-28T09:24:41Z

stan/math/fwd/fun/sum.hpp

 * @return Sum of vector entries.
 */
 template <typename T>
-inline fvar<T> sum(const std::vector<fvar<T>>& m) {
-  if (m.size() == 0) {
+inline fvar<T> sum(const std::vector<fvar<T>>& v) {


Actually, what kind of warnings did renaming a few variables solve?

t4c1

Looks good!. Just one question

SteveBronder · 2020-03-28T16:42:18Z

Actually, what kind of warnings did renaming a few variables solve?

It's something weird in doxygen

stan/math/rev/fun/multiply.hpp:545: warning: argument 'm1' of command @param is not found in the argument list of stan::math::multiply(const Mat1 &A, const Mat2 &B)

…e_generics and give correct includes to other headers

…stable/2017-11-14)

…cleanup/require_base

SteveBronder · 2020-03-28T17:08:04Z

@t4c1 fyi I just went through and fixed index and some header includes rest of the stuff is just merge and a cpplint fix

675b03d

stan-buildbot · 2020-03-28T22:55:35Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.95	4.96	1.0	-0.28% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.05	4.56% faster
eight_schools/eight_schools.stan	0.09	0.09	1.01	1.12% faster
gp_regr/gp_regr.stan	0.22	0.22	1.01	0.7% faster
irt_2pl/irt_2pl.stan	6.49	6.44	1.01	0.69% faster
performance.compilation	89.46	86.78	1.03	3.0% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.51	7.59	0.99	-1.03% slower
pkpd/one_comp_mm_elim_abs.stan	20.88	21.15	0.99	-1.3% slower
sir/sir.stan	94.01	94.05	1.0	-0.03% slower
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-0.66% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.96	2.95	1.0	0.23% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.32	0.32	1.0	0.32% faster
arK/arK.stan	1.78	1.74	1.03	2.48% faster
arma/arma.stan	0.66	0.66	1.0	-0.01% slower
garch/garch.stan	0.52	0.51	1.01	1.41% faster
Mean result: 1.00776504704

Jenkins Console Log
Blue Ocean
Commit hash: 878d3f4

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 and others added 25 commits March 17, 2020 19:42

add macros for defining basic requires

6e811e5

move value_type and scalar_type implementations for eigen and std vec…

24afc2c

…tor to is_eigen and is_std_vector

Add docs

8b7e05e

Adds more docs

ec51e13

docs

b18b233

Adds macro for value and scalar type of containers

94b532e

merge to develop

dd36f03

Fix container_st and remove DOI badge

b4c337a

add back zenodo badge

6e3b0ec

set WARN_AS_ERROR to YES in doxygen

cfc78fa

Update docs for macros

2eb07cc

Merge commit '6683303a1981545a0a95b6855eb31354897a016c' into HEAD

f086598

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

a0124b9

…gs/RELEASE_500/final)

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

e957a0c

…gs/RELEASE_500/final)

update cpplint

1a3bd5f

cpplint

f9169e3

Update docs

5c9dc76

fix includes

ed7952c

Move macros all to one file

28181e5

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

2a35586

…gs/RELEASE_500/final)

headers for is_autodiff

87abea7

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

f2b4413

…stable/2017-11-14)

include headers

6525065

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

f218fcd

…gs/RELEASE_500/final)

fix requires tests

ba5a07e

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

dfdfdba

…stable/2017-11-14)

t4c1 requested changes Mar 26, 2020

View reviewed changes

SteveBronder and others added 7 commits March 27, 2020 21:15

Merge remote-tracking branch 'origin/develop' into cleanup/require_base

0dcbc3b

condense scalar and value macros to one, update docs and revert previ…

0d1212b

…ous doc fixes

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

a53c42f

…stable/2017-11-14)

fix same requires

1f714e4

Merge branch 'cleanup/require_base' of github.com:stan-dev/math into …

c719795

…cleanup/require_base

fix require tests

c96ed5b

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

70035f4

…gs/RELEASE_500/final)

SteveBronder linked an issue Mar 28, 2020 that may be closed by this pull request

Breakup require_generics.hpp into multiple files #1739

Closed

t4c1 reviewed Mar 28, 2020

View reviewed changes

t4c1 previously approved these changes Mar 28, 2020

View reviewed changes

make require_index into require_integral. Remove includes from requir…

675b03d

…e_generics and give correct includes to other headers

SteveBronder dismissed t4c1’s stale review via 675b03d March 28, 2020 17:02

yashikno and others added 4 commits March 28, 2020 13:02

Merge commit '57e469aa1d04b9f53bc95fececb90e374e403db2' into HEAD

2dc1e92

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

169f47d

…stable/2017-11-14)

cpplint fix

ca861a5

Merge branch 'cleanup/require_base' of github.com:stan-dev/math into …

878d3f4

…cleanup/require_base

t4c1 approved these changes Mar 30, 2020

View reviewed changes

t4c1 merged commit 4649606 into develop Mar 30, 2020

SteveBronder mentioned this pull request Apr 16, 2020

Stan Math 3.2 release #1826

Closed

bbbales2 mentioned this pull request Apr 20, 2020

Stanc3 release for Cmdstan 2.23 stan-dev/stanc3#498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup requires with macros #1794

Cleanup requires with macros #1794

SteveBronder commented Mar 23, 2020 •

edited

Loading

SteveBronder commented Mar 24, 2020

serban-nicusor-toptal commented Mar 24, 2020

stan-buildbot commented Mar 24, 2020

stan-buildbot commented Mar 25, 2020

SteveBronder commented Mar 25, 2020

t4c1 Mar 26, 2020

SteveBronder Mar 26, 2020

SteveBronder Mar 28, 2020

t4c1 Mar 28, 2020

t4c1 Mar 28, 2020

t4c1 Mar 28, 2020

t4c1 Mar 26, 2020

SteveBronder Mar 28, 2020

t4c1 Mar 28, 2020

t4c1 Mar 28, 2020

stan-buildbot commented Mar 28, 2020

t4c1 Mar 28, 2020

SteveBronder Mar 28, 2020

t4c1 Mar 28, 2020

t4c1 left a comment

SteveBronder commented Mar 28, 2020

SteveBronder commented Mar 28, 2020

stan-buildbot commented Mar 28, 2020

Cleanup requires with macros #1794

Cleanup requires with macros #1794

Conversation

SteveBronder commented Mar 23, 2020 • edited Loading

Summary

Tests

Side Effects

Release notes

Checklist

SteveBronder commented Mar 24, 2020

serban-nicusor-toptal commented Mar 24, 2020

stan-buildbot commented Mar 24, 2020

stan-buildbot commented Mar 25, 2020

SteveBronder commented Mar 25, 2020

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

Choose a reason for hiding this comment

stan-buildbot commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t4c1 left a comment

Choose a reason for hiding this comment

SteveBronder commented Mar 28, 2020

SteveBronder commented Mar 28, 2020

stan-buildbot commented Mar 28, 2020

SteveBronder commented Mar 23, 2020 •

edited

Loading