Use Eigen's allFinite to cut execution time of normal_glm (and others?) by 27% #1005

seantalts · 2018-08-27T10:30:58Z

For background discussion, see this thread

Checklist

Copyright holder: (fill in copyright holder information)

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: Columbia University
- 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)
- 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 · 2018-08-27T10:42:16Z

Please don't just refer to a thread in a PR description unless it's for background or something. Here it looks like the reviewer needs to review that thread in order to find the intent of the PR. I think the title's fine, and this can say "for background discussions, see this thread" to make it clear it's not necessary to read that thread to understand the PR.

I don't think the class should be named stan::math::finite as it has a very specific purpose.

Also, shouldn't this just be rolled into our basic is_finite tests for the vector case so that we don't have to go and change all our code one-off?

seantalts · 2018-08-27T10:46:33Z

Fixed the text of the comment to make it clear that the thread was optional reading.

Unfortunately the class is already named that; this is just a specialization of it.

It is rolled into those checks via the template struct finite

bob-carpenter

Lots of changes requested for such a small PR!

bob-carpenter · 2018-08-27T10:43:07Z

stan/math/prim/mat/err/check_finite.hpp

+#define STAN_MATH_PRIM_MAT_ERR_CHECK_FINITE_HPP
+
+#include <Eigen/Dense>
+#include <stan/math/prim/scal/err/check_finite.hpp>


This is not being used.

bob-carpenter · 2018-08-27T10:44:26Z

stan/math/prim/mat/err/check_finite.hpp

+namespace math {
+namespace {
+template <typename T, int R, int C>
+struct finite<Eigen::Matrix<T, R, C>, true> {


I'd prefer the name stan::math::internal::is_finite to indicate the behavior of the class.

bob-carpenter · 2018-08-27T10:45:34Z

stan/math/prim/mat/err/check_finite.hpp

+  static void check(const char* function, const char* name,
+                    const Eigen::Matrix<T, R, C>& y) {
+    if (!y.allFinite())
+      domain_error(function, name, y, "is ", ", but must be finite!");


This degrades the error reporting so that it doesn't say which element. What does the print look like given that y is a matrix? Does it dump out the whole matrix?

What would you think of having a loop inside the test that picks out which element isn't finite? We don't care so much how long things take when they fail.

bob-carpenter · 2018-08-27T10:46:53Z

stan/math/prim/mat/err/check_finite.hpp

+
+namespace stan {
+namespace math {
+namespace {


We decided to use the internal namespace rather than anonymous namespaces because the anonymous namespaces didn't seem to do what they advertised (file private scope). Here, it looks like you're specializing another structure in another anonymous namespace!

seantalts · 2018-08-27T10:54:31Z

@bob-carpenter Would you mind reading my comment above about how finite is a pre-existing template struct (in that include file that is actually being used) and taking another look at the code with that information? Thanks.

bob-carpenter · 2018-08-27T11:01:40Z

I understood that from the form of the code that you were providing a template specialization. That's why I said the anonymous namespaces weren't working as advertised (file local) and it should be changed to internal to make the code clearer. Anyway, it's no big deal and if you want to leave the non-standard code in, the only thing I ask is that you open a new issue with the rest of my comments and assign it to yourself or Matthijs to fix going forward. I'm much more concerned about the lack of "internal" than I am about the name given that it's internal.

seantalts · 2018-08-27T11:07:35Z

I didn't choose the namespace, either - I believe it has to be in the same namespace as the template struct it's specializing, right?

I don't see why the refactoring you want is associated with this pull request - I'm forced into this code in order to provide this functionality by the existing code. I can't do any of your suggestions without a larger refactoring effort. If you want to change the existing structure of how is_finite and finite etc are implemented that sounds like a great thing to write an issue about but do you think it's worth holding up this performance fix for the sake of that refactor?

seantalts · 2018-08-27T11:12:54Z

I think it's harmful to have a policy to suggest that PRs fix all of the code and files that they merely touch or reference (versus just that the new code going in is a strict improvement upon the old). There are two reasons I believe that asking each PR to change all of its related code is harmful:

Obviously this increases the burden on contributors a lot, but more importantly
PRs are now much larger and mix many different intentions, so they become much more difficult to review and selectively merge or revert.

bbbales2 · 2018-08-27T11:31:11Z

I did a 3rd party review of this. I think the namespace thing needs to be a separate issue ("clean up anonymous namespaces in Stan"). These are clearly used elsewhere a lot.

The other two comments are worth addressing (unused include, error degradation)

seantalts · 2018-08-27T11:46:14Z

The unused include is used to refer to the original finite struct that it is specializing. It doesn't compile without that.

The error degradation I missed before. I can have a loop on failure that picks out which elements are not finite I suppose.

bbbales2 · 2018-08-27T11:49:34Z

Lol, overload vs. specialization, man it's easy to miss the difference

seantalts · 2018-08-27T13:32:19Z

I made an issue (#1006) and made the error message the same as in the previous code (thus also fixing the failing test, which looked for a specific error message). I think this is ready to go again (though feel free to wait until the tests pass).

bob-carpenter · 2018-08-28T06:05:44Z

Thanks for creating the issue to clean up namespaces. I'll finalize the review.

bob-carpenter

Looks good. Thanks for creating the separate issue for namespaces.

bob-carpenter · 2018-08-28T06:10:02Z

stan/math/prim/mat/err/check_finite.hpp

+    if (!value_of(y).allFinite()) {
+      for (int n = 0; n < y.size(); ++n) {
+        if (!(boost::math::isfinite)(y(n)))
+          domain_error_vec(function, name, y, n, "is ",


Thanks for refining the error message.

Another refactor we need to do is remove all these Boost things with the std lib. There's now std::isfinite in C++11.

seantalts · 2018-08-29T03:51:25Z

This is just waiting on a fix to the windows Jenkins box that's holding up all the tests.

bgoodri · 2018-08-29T05:45:01Z

Need to know full IP address to login

…

On Tue, Aug 28, 2018 at 11:51 PM seantalts ***@***.***> wrote: This is just waiting on a fix to the windows Jenkins box that's holding up all the tests. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1005 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOrqjbjDDbKsEPcvAyTremt8zwMOCnfks5uVg--gaJpZM4WNjaq> .

seantalts and others added 2 commits August 27, 2018 06:29

38% speed increase in normal GLM

469586f

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

59670cd

…stable/2017-11-14)

bob-carpenter requested changes Aug 27, 2018

View reviewed changes

Retain the same error message from vectorized version

ccc9f12

value_of is probably kinder to the autodiff stack.

7edbaf9

bob-carpenter approved these changes Aug 28, 2018

View reviewed changes

seantalts changed the title ~~Use Eigen's allFinite to cut execution time of normal_glm (and others?) by 38%~~ Use Eigen's allFinite to cut execution time of normal_glm (and others?) by 27% Aug 28, 2018

seantalts merged commit 1875575 into develop Aug 29, 2018

seantalts deleted the perf/isfinite branch August 29, 2018 21:09

peterwicksstringfield mentioned this pull request Mar 24, 2020

feature/elementwise_check (2) #1798

Merged

5 tasks

Uh oh!

Use Eigen's allFinite to cut execution time of normal_glm (and others?) by 27% #1005

Use Eigen's allFinite to cut execution time of normal_glm (and others?) by 27% #1005

Uh oh!

Conversation

seantalts commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

bob-carpenter commented Aug 27, 2018

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

bob-carpenter left a comment

Choose a reason for hiding this comment

Uh oh!

bob-carpenter Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

bob-carpenter Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

bob-carpenter Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

bob-carpenter Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

bob-carpenter commented Aug 27, 2018 via email

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

bbbales2 commented Aug 27, 2018

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

bbbales2 commented Aug 27, 2018

Uh oh!

seantalts commented Aug 27, 2018

Uh oh!

bob-carpenter commented Aug 28, 2018 via email

Uh oh!

bob-carpenter left a comment

Choose a reason for hiding this comment

Uh oh!

bob-carpenter Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

seantalts commented Aug 29, 2018

Uh oh!

bgoodri commented Aug 29, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seantalts commented Aug 27, 2018 •

edited

Loading