Reverse mode specializaiton for matrix add and subtract by dpsimpson · Pull Request #1812 · stan-dev/math

dpsimpson · 2020-03-31T15:03:38Z

Summary

This is addresses part of #1787 by adding reverse mode specializations for the matrix add and subtract functions. This should reduce the size of the autodiff stack.

The implementation uses a lot of tricks copped from the specialization of multiply.

subtract uses a few constexpr ifs to avoid re-writing code. I'm getting a C++17 warning, and I can change them to ordinary ifs if it's easier. They're there so I didn't have to implement different vari extensions for A - c and c - A (where A is a matrix and c is a scalar). I had honestly hoped I wouldn't need to write almost equivalent code for subtract(Scalar,Matrix) and subtract(Matrix, Scalar) (lines 549-600 of rev/fun/subtract.hpp) but I couldn't work out how to do that.

As always, any suggestions/substitutions/changes greatly appreciated.

Tests

The following tests are implemented for both add and subtract:

(double, double), (double, var), (var, double), (var, var) using random matrices.
Bound checking tests

I'm happy to add more, but I couldn't think of any.

Side Effects

This should be a plug in replacement.

Release notes

Replace this text with a short note on what will change if this pull request is merged in which case this will be included in the release notes.

Checklist

Math issue autodiff for matrix add and subtract #1787 (Partial)
Copyright holder: Daniel simpson

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

…stable/2017-11-14)

bob-carpenter · 2020-03-31T16:45:27Z

Thanks for writing tests, but they're probably largely redundant given the testing framework tests which cover all levels of autodiff against values and finite differences of the double implementation. For these functions, those are in:

test/unit/math/mix/fun/multiply_test.cpp
test/unit/math/mix/fun/add_test.cpp

The one thing that should be done is to add throw tests for mismatched sizes in multiply_test.cpp like the ones for add_test.cpp.

andrjohns · 2020-04-01T03:21:56Z

I'll take the review for this one (if nobody else had started)

andrjohns

Looks great! Some suggestions included. The main issue is the use of if constexpr which is above the C++ standard that Stan currently requires, so that will need to be re-written/replaced

andrjohns · 2020-04-01T02:43:22Z

@@ -0,0 +1,538 @@
+#ifndef STAN_MATH_REV_FUN_MATRIX_ADD_HPP
+#define STAN_MATH_REV_FUN_MATRIX_ADD_HPP


Shouldn't this be *_FUN_ADD_HPP (i.e. no 'MATRIX') for consistency with prim?

andrjohns · 2020-04-01T02:44:15Z

+#include <stan/math/rev/core.hpp>
+#include <stan/math/prim.hpp>
+#include <type_traits>
+#include <iostream>


These last two headers should be removed

andrjohns · 2020-04-01T02:50:54Z

+   * vari's constructor.
+   *
+   * @param A matrix
+   * @param  B matrix


Suggested change

* @param B matrix

* @param B matrix

andrjohns · 2020-04-01T02:52:40Z

+   * vari's constructor.
+   *
+   * @param A matrix
+   * @param  B matrix


Suggested change

* @param B matrix

* @param B matrix

andrjohns · 2020-04-01T03:22:26Z

+ * The class stores the structure of each matrix,
+ * the double values of A and B, and pointers to
+ * the varis for A and B if A or B is a var. It
+ * also instatiates and stores pointers to varis


Suggested change

* also instatiates and stores pointers to varis

* also instantiates and stores pointers to varis

andrjohns · 2020-04-01T03:24:29Z

+    matrix_d adjAminusB(rows_, cols_);
+    adjAminusB = Map<matrix_vi>(variRefAminusB_, rows_, cols_).adj();
+    Map<matrix_vi>(variRefB_, rows_, cols_).adj() -= adjAminusB;


Suggested change

matrix_d adjAminusB(rows_, cols_);

adjAminusB = Map<matrix_vi>(variRefAminusB_, rows_, cols_).adj();

Map<matrix_vi>(variRefB_, rows_, cols_).adj() -= adjAminusB;

Map<matrix_vi> AminusB(variRefAminusB_, rows_, cols_);

Map<matrix_vi>(variRefB_, rows_, cols_).adj() -= AminusB.adj();

andrjohns · 2020-04-01T03:27:22Z

+    Map<matrix_d> Ad(Ad_, rows_, cols_);
+    double cd = c.val();
+    Ad = A.val();
+    if constexpr (MatMinusScal) {


if constexpr is c++17, and we're only up to c++14

Also, see #1300 for more info

One of these days!!!

andrjohns · 2020-04-01T03:29:34Z

+ * @tparam Scal type of the scalar
+ * @tparam Mat type of the matrix or expression
+ * @param c Scalar.
+ * @param m Matrix.


Suggested change

* @param m Matrix.

* @param A Matrix.

andrjohns · 2020-04-01T03:32:29Z

+    double cd = c.val();
+    Map<matrix_d> Ad(Ad_, rows_, cols_);
+    Ad = A.val();
+    Map<matrix_vi>(variRefAplusc_, rows_, cols_)
+        = (Ad.array() + cd).matrix().unaryExpr([](double x) {
+            return new vari(x, false);
+          });


Same suggested changes as the above

bbbales2 · 2020-04-06T20:48:45Z

@dpsimpson How did this compare in the end to the un-autodiffed version?

dpsimpson · 2020-04-06T20:50:19Z

Sorry - got caught up in a work thing. Will do stuff tonight and tomorrow.

…

On Mon, 6 Apr 2020 at 16:49, Ben Bales ***@***.***> wrote: @dpsimpson <https://github.com/dpsimpson> How did this compare in the end to the un-autodiffed version? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1812 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRICBTJ7UISYXNXMBBTQB3RLI533ANCNFSM4LXUAOVA> .

wds15 · 2020-04-13T18:09:50Z

This will be in 2.24? Or can this move in today?

SteveBronder · 2020-04-13T18:59:55Z

I think this would be a new feature so it will be in the next release

wds15 · 2020-04-13T19:11:25Z

Of course...this is new stuff. The pr comments sounded as if this is about ready. I expect this pr to speed up things quite a bit which is why I was bumping this for some attention.

But let’s wait for the next release.

dpsimpson · 2020-04-13T19:37:55Z

Definitely next release - swamped with exam stuff. Will pick it up ASAP. Sorry.

…

On Mon, Apr 13, 2020 at 15:11 wds15 ***@***.***> wrote: Of course...this is new stuff. The pr comments sounded as if this is about ready. I expect this pr to speed up things quite a bit which is why I was bumping this for some attention. But let’s wait for the next release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1812 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRICBSKT4CP34L3T54EHSTRMNPWZANCNFSM4LXUAOVA> .

SteveBronder · 2020-07-16T14:49:10Z

Closing this for now but feel free to open when you have time to fixup!

spinkney · 2023-02-03T03:17:54Z

@dpsimpson any chance this can get resurrected?

dpsimpson · 2023-02-03T04:29:35Z

I thought it was superseded. But absolutely! I’ll have a look over the weekend.

…

On Fri, Feb 3, 2023 at 14:18 Sean Pinkney ***@***.***> wrote: @dpsimpson <https://github.com/dpsimpson> any chance this can get resurrected? — Reply to this email directly, view it on GitHub <#1812 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRICBUVWKTVHXIUUHYZBJLWVR2G5ANCNFSM4LXUAOVA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dpsimpson and others added 3 commits March 31, 2020 02:46

Reverse mode specializaiton for matrix add and subtract

ec91342

Merge commit 'afe318dc8eb7cfef08509fb8ce16dcf94e3a0ab9' into HEAD

a209990

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

7c71f39

…stable/2017-11-14)

andrjohns requested changes Apr 1, 2020

View reviewed changes

SteveBronder closed this Jul 16, 2020

		@@ -0,0 +1,538 @@
		#ifndef STAN_MATH_REV_FUN_MATRIX_ADD_HPP
		#define STAN_MATH_REV_FUN_MATRIX_ADD_HPP

	* also instatiates and stores pointers to varis
	* also instantiates and stores pointers to varis

Uh oh!

Uh oh!

Conversation

dpsimpson commented Mar 31, 2020

Summary

Tests

Side Effects

Release notes

Checklist

Uh oh!

bob-carpenter commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrjohns commented Apr 1, 2020

Uh oh!

andrjohns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbbales2 commented Apr 6, 2020

Uh oh!

dpsimpson commented Apr 6, 2020 via email

Uh oh!

wds15 commented Apr 13, 2020

Uh oh!

SteveBronder commented Apr 13, 2020

Uh oh!

wds15 commented Apr 13, 2020

Uh oh!

dpsimpson commented Apr 13, 2020 via email

Uh oh!

SteveBronder commented Jul 16, 2020

Uh oh!

spinkney commented Feb 3, 2023

Uh oh!

dpsimpson commented Feb 3, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bob-carpenter commented Mar 31, 2020 •

edited

Loading