Remove linear indexing requirements from functions #2205

t4c1 · 2020-11-23T12:02:07Z

Summary

Modifies functions requiring linear indexing of their arguments to remove that requirement.

This reintroduces part of functionality reverted in #1982.

Tests

Expression tests are modified to use a block expression, which does not support linear indexing so the tests will fail for any function that still requires linear indexing.

Side Effects

None.

Release notes

Modified functions requiring linear indexing of their arguments to remove that requirement.

Checklist

Math issue Generalize matrix function signatures #1470
Copyright holder: Tadej Ciglarič

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)

t4c1 · 2020-11-23T12:04:15Z

@bbbales2 You found the performance regression that lead to the revert #1982, but I did not find what exactly were you testing. Can you check that this PR does not cause a similar regression or post the code you used for the benchmark?

bbbales2 · 2020-11-23T14:01:14Z

@t4c1 this revert has a math benchmark: #2147

This is the model from the July revert: https://discourse.mc-stan.org/t/cmdstan-2-24-release-candidate-now-available/16818/37 (timings further down)

bbbales2 · 2020-11-23T14:03:24Z

Also I'm not gonna complain if you implement all the functionality for #1982 :P. It would be convenient to have all these checks work on nested structures.

…4.1 (tags/RELEASE_600/final)

…statcomp/math into no_linear_indexing_requirement

t4c1 · 2020-11-24T11:03:42Z

Thanks Ben. I ran those and fixed the code, so now it is as fast as develop. It turns out elementwise_check was too big and g++ decided not to inline it.

stan/math/prim/fun/csr_u_to_z.hpp

bbbales2 · 2020-11-24T11:34:59Z

stan/math/prim/err/elementwise_check.hpp

@@ -203,7 +196,7 @@ inline void elementwise_check(const F& is_good, const char* function,
                              const Indexings&... indexings) {
  for (size_t j = 0; j < x.size(); j++) {
    elementwise_check(is_good, function, name, x[j], must_be, indexings..., "[",
-                      j, "]");
+                      j + 1, "]");


Why the + 1 here?

Does this have anything to do with Stan indexing by 1 or not? There's a variable for this here: https://github.com/stan-dev/math/blob/235090a31ccdabdd0d9d35bfb77ba3d4b984590f/stan/math/prim/meta/error_index.hpp

I think by default the Math library compiles as if it is indexing by 1, and you can optionally change this variable.

Because I removed the +1 from internal::pipe_in. Yeah I can change this to instead use + error_index::value.

…4.1 (tags/RELEASE_600/final)

…pose

stan-buildbot · 2020-11-27T19:14:27Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.55	3.57	0.99	-0.51% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.92	-8.27% slower
eight_schools/eight_schools.stan	0.12	0.12	0.99	-0.73% slower
gp_regr/gp_regr.stan	0.17	0.17	1.0	0.5% faster
irt_2pl/irt_2pl.stan	5.67	5.66	1.0	0.15% faster
performance.compilation	87.06	85.6	1.02	1.69% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.45	8.44	1.0	0.19% faster
pkpd/one_comp_mm_elim_abs.stan	29.36	29.28	1.0	0.26% faster
sir/sir.stan	134.04	127.48	1.05	4.89% faster
gp_regr/gen_gp_data.stan	0.04	0.04	0.98	-1.8% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.93	2.95	0.99	-0.7% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.37	0.39	0.93	-7.45% slower
arK/arK.stan	2.47	2.45	1.01	1.0% faster
arma/arma.stan	0.6	0.59	1.01	1.21% faster
garch/garch.stan	0.75	0.74	1.01	0.81% faster
Mean result: 0.995161552191

Jenkins Console Log
Blue Ocean
Commit hash: 0d5cef3

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-11-28T11:18:25Z

This is ready for review.

bbbales2

Review!

test/expressions/generateExpressionTests.py

stan/math/rev/core/arena_matrix.hpp

test/unit/math/prim/prob/neg_binomial_2_test.cpp

stan/math/prim/err/elementwise_check.hpp

stan-buildbot · 2020-11-28T16:09:25Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.53	3.52	1.0	0.27% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.22% slower
eight_schools/eight_schools.stan	0.11	0.12	0.97	-3.03% slower
gp_regr/gp_regr.stan	0.17	0.16	1.02	1.87% faster
irt_2pl/irt_2pl.stan	5.73	5.65	1.01	1.45% faster
performance.compilation	86.96	85.72	1.01	1.43% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.46	8.42	1.0	0.43% faster
pkpd/one_comp_mm_elim_abs.stan	30.86	28.75	1.07	6.83% faster
sir/sir.stan	137.7	127.05	1.08	7.73% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.91	-10.0% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.94	2.93	1.0	0.27% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.41	0.4	1.02	2.4% faster
arK/arK.stan	2.49	2.45	1.02	1.89% faster
arma/arma.stan	0.6	0.59	1.02	2.21% faster
garch/garch.stan	0.74	0.74	1.0	0.48% faster
Mean result: 1.00963060401

Jenkins Console Log
Blue Ocean
Commit hash: 0d5cef3

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-11-30T13:46:56Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.69	3.56	1.04	3.54% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.04	4.28% faster
eight_schools/eight_schools.stan	0.11	0.11	1.0	0.42% faster
gp_regr/gp_regr.stan	0.17	0.16	1.01	1.36% faster
irt_2pl/irt_2pl.stan	5.87	5.8	1.01	1.14% faster
performance.compilation	86.87	85.7	1.01	1.34% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.46	8.37	1.01	1.04% faster
pkpd/one_comp_mm_elim_abs.stan	30.45	28.84	1.06	5.3% faster
sir/sir.stan	132.59	126.9	1.04	4.3% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.96	-4.15% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.94	2.92	1.01	0.72% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.37	0.39	0.96	-4.36% slower
arK/arK.stan	2.48	2.44	1.01	1.29% faster
arma/arma.stan	0.6	0.72	0.83	-20.46% slower
garch/garch.stan	0.55	0.55	0.99	-0.54% slower
Mean result: 0.999967802476

Jenkins Console Log
Blue Ocean
Commit hash: 27b17be

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

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-11-30T14:28:57Z

@t4c1 looks good. Will approve when tests pass

stan-buildbot · 2020-11-30T19:20:19Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.57	3.48	1.03	2.61% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.86% slower
eight_schools/eight_schools.stan	0.11	0.11	0.98	-1.63% slower
gp_regr/gp_regr.stan	0.16	0.16	1.0	0.35% faster
irt_2pl/irt_2pl.stan	5.82	5.84	1.0	-0.25% slower
performance.compilation	86.9	85.48	1.02	1.63% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.43	8.4	1.0	0.27% faster
pkpd/one_comp_mm_elim_abs.stan	29.03	28.64	1.01	1.35% faster
sir/sir.stan	133.35	129.81	1.03	2.66% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.99	-1.09% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	2.95	1.0	-0.03% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.4	0.97	-3.07% slower
arK/arK.stan	2.47	2.45	1.01	0.99% faster
arma/arma.stan	0.6	0.73	0.82	-21.8% slower
garch/garch.stan	0.54	0.55	0.98	-2.02% slower
Mean result: 0.987651638077

Jenkins Console Log
Blue Ocean
Commit hash: e3fbd53

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 and others added 3 commits November 23, 2020 12:52

simplify and use elementwise_check helper

61160ed

test that no function requires linear indexing

eadb6fa

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

88af7c5

…4.1 (tags/RELEASE_600/final)

t4c1 mentioned this pull request Nov 23, 2020

Let prim functions return expressions #2190

Merged

5 tasks

t4c1 added 7 commits November 23, 2020 13:08

handle autodiff types

6a28b3d

fixed doxygen

14b2601

added missing header

8e0d5cf

fix a typo and more includes

81434c1

typo

b97a903

back to value_of_rec

1501d93

fix typo

b14b350

t4c1 and others added 8 commits November 24, 2020 09:27

added Eigen traits for arena_matrix

20cc034

typos

9cf5ceb

Merge commit '92fce0218c9fb15fd405ef031f488cad05c5546b' into HEAD

4406ad7

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

d703bcd

…4.1 (tags/RELEASE_600/final)

another typo

7d48486

Merge branch 'no_linear_indexing_requirement' of https://github.com/b…

40e849c

…statcomp/math into no_linear_indexing_requirement

fix performance regression

bf508ba

fix some brittle tests

d713cd7

bugfix

ede1749

bbbales2 reviewed Nov 24, 2020

View reviewed changes

t4c1 added 5 commits November 24, 2020 15:26

bugfix

86c7bea

replace +1 with +error_index::value

ec1f046

added space and improved some brittle tests

e035d83

another brittle test

1826702

another testimproved

55ffbbe

stan-buildbot and others added 4 commits November 26, 2020 08:10

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

60e204e

…4.1 (tags/RELEASE_600/final)

bugfix expression tests

2e169a2

remove linear indexing requirement from multiply_lower_tri_self_trans…

2ba50b6

…pose

remove linear indexing requirement from to_matrix

0d5cef3

t4c1 changed the title ~~WIP: no linear indexing requirement~~ Remove linear indexing requirements from functions Nov 28, 2020

bbbales2 requested changes Nov 28, 2020

View reviewed changes

addressed review comments

27b17be

t4c1 and others added 3 commits November 30, 2020 15:15

applied Ben's suggestion

ddfaa12

Merge commit '0c1ef70578684386fae2b15af26d2d3854790959' into HEAD

3e27038

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

e3fbd53

…4.1 (tags/RELEASE_600/final)

bbbales2 approved these changes Dec 1, 2020

View reviewed changes

bbbales2 merged commit 9d3cd07 into stan-dev:develop Dec 1, 2020

bbbales2 mentioned this pull request Dec 7, 2020

Cleaning up linear access functions #2245

Open

t4c1 mentioned this pull request Dec 10, 2020

Use immedietly invoked lambdas to make error checking less expensive #2249

Closed

t4c1 deleted the no_linear_indexing_requirement branch January 18, 2021 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove linear indexing requirements from functions #2205

Remove linear indexing requirements from functions #2205

t4c1 commented Nov 23, 2020 •

edited

Loading

t4c1 commented Nov 23, 2020

bbbales2 commented Nov 23, 2020

bbbales2 commented Nov 23, 2020

t4c1 commented Nov 24, 2020

bbbales2 Nov 24, 2020

t4c1 Nov 24, 2020

stan-buildbot commented Nov 27, 2020

t4c1 commented Nov 28, 2020

bbbales2 left a comment

stan-buildbot commented Nov 28, 2020

stan-buildbot commented Nov 30, 2020

bbbales2 commented Nov 30, 2020

stan-buildbot commented Nov 30, 2020

Remove linear indexing requirements from functions #2205

Remove linear indexing requirements from functions #2205

Conversation

t4c1 commented Nov 23, 2020 • edited Loading

Summary

Tests

Side Effects

Release notes

Checklist

t4c1 commented Nov 23, 2020

bbbales2 commented Nov 23, 2020

bbbales2 commented Nov 23, 2020

t4c1 commented Nov 24, 2020

bbbales2 Nov 24, 2020

Choose a reason for hiding this comment

t4c1 Nov 24, 2020

Choose a reason for hiding this comment

stan-buildbot commented Nov 27, 2020

t4c1 commented Nov 28, 2020

bbbales2 left a comment

Choose a reason for hiding this comment

stan-buildbot commented Nov 28, 2020

stan-buildbot commented Nov 30, 2020

bbbales2 commented Nov 30, 2020

stan-buildbot commented Nov 30, 2020

t4c1 commented Nov 23, 2020 •

edited

Loading