Basic slicing and dimension queries operators support var<Matrix> types #2507

SteveBronder · 2021-06-15T22:41:16Z

Summary

As title says, this adds slicing and dimension queries like rows() and cols() for var_value<Eigen> types

Tests

Tests can be run with

./runTests.py ./test/unit/math/rev/fun/dimensions_test.cpp \
 ./test/unit/math/rev/fun/cols_test.cpp \
 ./test/unit/math/rev/fun/rows_test.cpp

I put most of them in dimensions_test because they are all quite small and similar

Side Effects

Are there any side effects that we should be aware of?

Release notes

Adds support for basic slicing and dimension queries operators support var types

Checklist

Math issue How to add static matrix? #1805
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

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2021-06-16T05:09:58Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.02	3.16	0.95	-4.79% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.0	-0.25% slower
eight_schools/eight_schools.stan	0.11	0.11	1.0	0.04% faster
gp_regr/gp_regr.stan	0.16	0.16	1.02	1.59% faster
irt_2pl/irt_2pl.stan	5.92	5.9	1.0	0.34% faster
performance.compilation	88.13	86.79	1.02	1.52% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.79	8.7	1.01	1.05% faster
pkpd/one_comp_mm_elim_abs.stan	31.49	29.6	1.06	6.01% faster
sir/sir.stan	123.31	120.31	1.02	2.43% faster
gp_regr/gen_gp_data.stan	0.03	0.03	1.0	-0.06% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.0	3.01	1.0	-0.38% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.4	0.4	1.0	-0.12% slower
arK/arK.stan	2.56	2.53	1.01	1.08% faster
arma/arma.stan	0.63	0.64	0.98	-1.75% slower
garch/garch.stan	0.68	0.68	1.01	0.68% faster
Mean result: 1.0054437949

Jenkins Console Log
Blue Ocean
Commit hash: 63618a1

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 · 2021-06-16T07:49:35Z

stan/math/rev/core/vari.hpp

+  vari_view(S&& val, K&& adj) noexcept
+      : val_(std::forward<S>(val)), adj_(std::forward<K>(adj)) {}


This does not seem to be in line with other changes in this PR. What is the reason for this change? I dont think we ever have views that would need forwarding.

Whups, yeah this should not be changed. Was debugging something and thought this was a point of error but it's fine as it was

t4c1 · 2021-06-16T07:56:37Z

test/unit/math/rev/fun/dimensions_test.cpp

+  var_value<Eigen::VectorXd> vec_v(vec);
+  EXPECT_MATRIX_FLOAT_EQ(stan::math::segment(vec_v, 2, 2).val(),
+                         stan::math::segment(vec, 2, 2));
+}


[optional] To be consistent with the rest of math the tests for these functions should be each in a file named after the function it is testing / matching the hpp that defines the function. Otherwise they are hard to find.

Also I did not see any tests for check* function you changed. Given how minimal these changes are I would not require the tests, but they would be nice to have.

I'm fine with breaking them up into multiple functions, it's mostly that they are all very small and have nearly the same pattern so I thought it might be better just to put them in one file. Mostly so then when the jenkins is putting all the tests together into jumbo units it doesn't see 10 different files and just sees 1

Actually would you mind if I left these? We do something similar in vari_test as well and I think it's nicer for Jenkins if we keep them together

I don't like the small files either, but breaking the pattern makes it harder to find things. If you insist I am fine with leaving this as one file.

stan-buildbot · 2021-06-17T05:57:41Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.19	3.1	1.03	2.59% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.96	-4.41% slower
eight_schools/eight_schools.stan	0.11	0.11	1.01	1.17% faster
gp_regr/gp_regr.stan	0.16	0.16	1.0	-0.46% slower
irt_2pl/irt_2pl.stan	5.95	5.91	1.01	0.81% faster
performance.compilation	89.08	86.68	1.03	2.69% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.68	8.75	0.99	-0.8% slower
pkpd/one_comp_mm_elim_abs.stan	30.31	30.88	0.98	-1.88% slower
sir/sir.stan	124.72	125.83	0.99	-0.88% slower
gp_regr/gen_gp_data.stan	0.03	0.04	0.98	-1.64% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.02	3.0	1.01	0.63% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.39	0.99	-1.11% slower
arK/arK.stan	2.55	2.54	1.0	0.23% faster
arma/arma.stan	0.64	0.64	1.0	0.2% faster
garch/garch.stan	0.68	0.68	1.01	0.9% faster
Mean result: 0.999004484422

Jenkins Console Log
Blue Ocean
Commit hash: c3b0c30

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 3 commits June 15, 2021 18:38

Basic slicing and dimension queries operators support var<Matrix> types

53e8784

Merge commit '1d0b4a08b4a800a3e8d2c1a3559fbe9390b7584e' into HEAD

885a40a

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

63618a1

…4.1 (tags/RELEASE_600/final)

SteveBronder requested a review from t4c1 June 16, 2021 06:39

t4c1 requested changes Jun 16, 2021

View reviewed changes

SteveBronder added 2 commits June 16, 2021 12:10

change back vari_view constructor

e6fca4e

change back vari_view constructor

c3b0c30

t4c1 approved these changes Jun 17, 2021

View reviewed changes

SteveBronder merged commit da36ba4 into develop Jun 17, 2021

SteveBronder deleted the feature/simple-varmat-funs1 branch August 17, 2021 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic slicing and dimension queries operators support var<Matrix> types #2507

Basic slicing and dimension queries operators support var<Matrix> types #2507

SteveBronder commented Jun 15, 2021

stan-buildbot commented Jun 16, 2021

t4c1 Jun 16, 2021

SteveBronder Jun 16, 2021

t4c1 Jun 16, 2021

SteveBronder Jun 16, 2021

SteveBronder Jun 16, 2021

t4c1 Jun 16, 2021

stan-buildbot commented Jun 17, 2021

		vari_view(S&& val, K&& adj) noexcept
		: val_(std::forward<S>(val)), adj_(std::forward<K>(adj)) {}

Basic slicing and dimension queries operators support var<Matrix> types #2507

Basic slicing and dimension queries operators support var<Matrix> types #2507

Conversation

SteveBronder commented Jun 15, 2021

Summary

Tests

Side Effects

Release notes

Checklist

stan-buildbot commented Jun 16, 2021

t4c1 Jun 16, 2021

Choose a reason for hiding this comment

SteveBronder Jun 16, 2021

Choose a reason for hiding this comment

t4c1 Jun 16, 2021

Choose a reason for hiding this comment

SteveBronder Jun 16, 2021

Choose a reason for hiding this comment

SteveBronder Jun 16, 2021

Choose a reason for hiding this comment

t4c1 Jun 16, 2021

Choose a reason for hiding this comment

stan-buildbot commented Jun 17, 2021