fix returned expressions #1644

t4c1 · 2020-01-27T12:08:09Z

Summary

PR #1471 generalized some functions to return Eigen expressions. In #1643 it was found that the change prevents compilation of some models. This PR fixes the problem by making those functions return Eigen::Matrix again.

Tests

No tests, as this kind of problem is hard to test in general. But now I know this is an issue It will be simple not to repeat it.

Side Effects

Maybe the code will be a bit slower, but the difference should be less than the speedup introduced in #1471, if it is present at all.

Checklist

Math issue Stan Math functions should not return Eigen expressions #1643
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)
- 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)

t4c1 · 2020-01-27T12:26:58Z

@avehtari Can you check that using this branch fixes the problem?

avehtari · 2020-01-27T12:56:05Z

Seems to work.
bin/stanc --version gives stanc acfb4612 (Unix) and cmdstanr using that compiles and runs the longest example in that issue.

rok-cesnovar · 2020-01-27T13:51:33Z

@serban-nicusor-toptal just tagging you that the merge commit of this PR should be cherry-picked to master and released as Stan Math 3.1.1

This has to first pass tests. Let me know if you need any help regarding the patch release.

bob-carpenter · 2020-01-27T13:54:49Z

The code that failed to compile should be added as a test. An alternative is to add the problematic model to the good models in the stan-dev/stan repo.

avehtari · 2020-01-27T14:05:09Z

The code that failed to compile should be added as a test. An alternative is to add the problematic model to the good models in the stan-dev/stan repo.

Additional info: That model is our (Aki, Juho Piironen) recommended reference code for logistic regression with regularized horseshoe prior. This will be also included in posteriordb as with some data sets, we get very nice examples of high dimensional multimodal distributions for which dynamic HMC in Stan works well.

t4c1 · 2020-01-27T14:27:38Z

The code that failed to compile should be added as a test. An alternative is to add the problematic model to the good models in the stan-dev/stan repo.

A test like this will only test a single combination of nested function calls a(b(...)). The problem could potentially happen again for any combination of functions a and b from stan math, where b can return and a can accept Eigen::Matrix. If we want to make tests in a way that will prevent this from happening again all combinations should be tested. And I would prefere not to write these tests manually ...

However not testing that is not that large problem, since avoiding this is easy once one knows it could be an issue. Also once all functions are generalized to accept any Eigen expressions this issue can not happen again.

bob-carpenter · 2020-01-27T15:34:11Z

I understand that it's hard to test in full generality. Rather than waiting for an automated general solution, I'd like to see the tests added for the cases being fixed to at least ensure the problem is fixed and there are no regressions.

SteveBronder · 2020-01-27T22:11:46Z

Having no testing for this because we can't test everything seems like not a great answer

rok-cesnovar · 2020-01-28T07:51:22Z

That model is our (Aki, Juho Piironen) recommended reference code for logistic regression with regularized horseshoe prior.

I will make a Stan PR to add this to the good-models. Do I have your permission to add it @avehtari? If yes, I will add you both as copyright holder for the PR. Do you want to add any description as a commented-out line in the stan model?

avehtari · 2020-01-28T08:09:55Z

You can add the code.

Useful comment would be

The regularized horseshoe prior code from Appendix C.1 of Piironen and Vehtari (2017). Sparsity information and regularization in the horseshoe and other shrinkage priors. In Electronic Journal of Statistics, 11(2):5018-5051. https://projecteuclid.org/euclid.ejs/1513306866

as this paper and the appendix explains what that model is.

As the code is very direct implementation of math in that paper, it's likely that someone else writing the code just using the equations would get the same result and thus there is no creativity and it should not be copyrightable. I think that comment should be sufficient, but I and Juho don't mind if you want to have us as the copyright holders in the PR.

rok-cesnovar · 2020-01-28T19:28:33Z

Added Aki's and Juho's model to Stan and it fails on current math develop and passes with this PR as Math submodule. Once this PR is green we can proceed then.

failing:
https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/PR-2882/3/pipeline
passing:
https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/PR-2882/4/pipeline/
parameters of the passing: https://jenkins.mc-stan.org/job/Stan/job/PR-2882/4/parameters/

bob-carpenter · 2020-01-28T19:58:38Z

On Jan 28, 2020, at 2:28 PM, Rok Češnovar ***@***.***> wrote: Added Aki's and Juho's model to Stan and it fails on current math develop and passes with this PR as Math submodule.

Thanks!

SteveBronder · 2020-01-29T02:25:49Z

Should we just revert #1471 and #1558, do a hotfix release as 1.22.1 and make revert revert PRs for both of those? We need to do a 1.22.1 for cmdstan anyway

I think we went around this the opposite way we should have. We should have the stan repo first accept generic eigen expressions, then do this in math

andrjohns · 2020-01-29T02:31:17Z

stan/math/prim/fun/log_softmax.hpp

@@ -41,7 +41,7 @@ template <typename T, require_t<std::is_arithmetic<scalar_type_t<T>>>...>
 inline auto log_softmax(const T& x) {
  return apply_vector_unary<T>::apply(x, [&](const auto& v) {
    check_nonzero_size("log_softmax", "v", v);
-    return (v.array() - log_sum_exp(v)).matrix();
+    return (v.array() - log_sum_exp(v)).matrix().eval();


Rather than adding .eval() to each of these functions, you could instead modify the apply_vector_unary header to eval the eigen returns:

template <typename F> static inline auto apply(const T& x, const F& f) { return f(x).eval(); }

I went this way, so we don't need to first eval and than copy matrices in std::vector overloads that assign result of apply_vector_unary::apply() instead of returning it.

rok-cesnovar · 2020-01-29T07:56:21Z

Should we just revert #1471 and #1558, do a hotfix release as 1.22.1 and make revert revert PRs for both of those? We need to do a 1.22.1 for cmdstan anyway

We need to do 3.1.1 with either this PR or revert of #1471. I am fine either way. The main benefit of #1471 will be seen after we have everything using eigen expressions anyways I think.

t4c1 · 2020-01-29T08:00:34Z

Actually the main benefit of #1471 is using Eigens's implementations of exp that is way faster than std implementation.

rok-cesnovar · 2020-01-29T08:02:48Z

Reverting #1471 might however be an issue due to the flatten.

SteveBronder · 2020-01-29T14:45:29Z

Why do the test values need changed?

SteveBronder · 2020-01-29T14:47:01Z

I'll try the revert tonight

t4c1 · 2020-01-29T14:59:51Z

Why do the test values need changed?

At first I just copy-pasted same values for all functions. But it turns out for some functions they were out of range where functions are defined.

bob-carpenter · 2020-01-29T15:33:17Z

One possible fix for this kind of thing is define a function that composes a variable transform with the function being tested so that it can take unconstrained input values to test.

…

On Jan 29, 2020, at 9:59 AM, Tadej Ciglarič ***@***.***> wrote: Why do the test values need changed? At first I just copy-pasted same values for all functions. But it turns out for some functions they were out of range where functions are defined. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

rok-cesnovar · 2020-01-29T15:54:28Z

I'll try the revert tonight

We either add the merge commit of this PR to master or the revert merge commit PR to master and that becomes Math 3.1.1. Not sure its worth another set of Jenkins tests.

I am also not sure what you mean by

We should have the stan repo first accept generic eigen expressions

There is no issue in the Stan repo. Can you explain?

SteveBronder · 2020-01-29T16:51:49Z

We should have the stan repo first accept generic eigen expressions

There is no issue in the Stan repo. Can you explain?

In #1628 We hit some errors upstream in the Stan repo related to passing arbitrary eigen expressions.

I'm working on the indexing functions right now but we should check what else we would want to make sure takes in Eigen matrices. We'll probably need to do this .eval() on all the returns first while we get the signatures lined up. Then have one repo where we remove all the .evals()

SteveBronder · 2020-01-29T16:52:21Z

If all checks are passing and this fixes all the upstream issues then let's approve and merge

rok-cesnovar · 2020-01-29T16:59:45Z

We'll probably need to do this .eval() on all the returns first while we get the signatures lined up. Then have one repo where we remove all the .evals()

Yep, I see no other way of doing this. We also need to come up with a "eigen expression mother model" that would make sure all functions signatures that accept eigen matrices, then work with eigen expressions.

If all checks are passing and this fixes all the upstream issues then let's approve and merge

It does yes, and I have reviewed changes here and they look fine. I will however go through #1471 and this PR manually to double check no function from the first PR now returns eigen expressions. Then I will merge and that is where @serban-nicusor-toptal comes in :)

rok-cesnovar

Looks good.

fix returned expressions # Conflicts: # stan/math/prim/fun/log_softmax.hpp

t4c1 and others added 2 commits January 27, 2020 13:00

fixes returned expressions

70d9c21

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

58586ef

…stable/2017-11-14)

t4c1 changed the title ~~fixes returned expressions~~ fix returned expressions Jan 27, 2020

another case

b8da27c

fix minus

88cd965

added a test.

dfc4384

t4c1 added 5 commits January 28, 2020 09:19

added remaining tests

1cf5579

fix copy-paste

b7e741b

fix missing includes

5b846f6

another one

1ab15c7

one missed instance of the bug

7f277d6

rok-cesnovar mentioned this pull request Jan 28, 2020

Add logistic regression with reg. horseshoe prior to test eigen expression returns stan-dev/stan#2882

Merged

3 tasks

another one

f821a72

fixed atanh input values

5f7b2a9

andrjohns reviewed Jan 29, 2020

View reviewed changes

fix some more test input values

9689cb6

andrjohns mentioned this pull request Jan 29, 2020

Add reverse #1650

Merged

5 tasks

rok-cesnovar approved these changes Jan 29, 2020

View reviewed changes

rok-cesnovar merged commit 987ffae into stan-dev:develop Jan 29, 2020

rok-cesnovar added a commit that referenced this pull request Jan 29, 2020

Merge pull request #1644 from bstatcomp/fix_returned_expressions

a66987f

fix returned expressions # Conflicts: # stan/math/prim/fun/log_softmax.hpp

rok-cesnovar mentioned this pull request Jan 29, 2020

Release of Math 3.1.1 #1659

Closed

t4c1 mentioned this pull request Feb 19, 2020

Generalize view and size functions #1660

Merged

5 tasks

t4c1 deleted the fix_returned_expressions branch November 30, 2020 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix returned expressions #1644

fix returned expressions #1644

t4c1 commented Jan 27, 2020 •

edited

Loading

t4c1 commented Jan 27, 2020

avehtari commented Jan 27, 2020

rok-cesnovar commented Jan 27, 2020 •

edited

Loading

bob-carpenter commented Jan 27, 2020 •

edited

Loading

avehtari commented Jan 27, 2020

t4c1 commented Jan 27, 2020

bob-carpenter commented Jan 27, 2020 via email

SteveBronder commented Jan 27, 2020

rok-cesnovar commented Jan 28, 2020 •

edited

Loading

avehtari commented Jan 28, 2020

rok-cesnovar commented Jan 28, 2020

bob-carpenter commented Jan 28, 2020 via email

SteveBronder commented Jan 29, 2020

andrjohns Jan 29, 2020

t4c1 Jan 29, 2020

rok-cesnovar commented Jan 29, 2020

t4c1 commented Jan 29, 2020

rok-cesnovar commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

t4c1 commented Jan 29, 2020

bob-carpenter commented Jan 29, 2020 via email

rok-cesnovar commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

rok-cesnovar commented Jan 29, 2020

rok-cesnovar left a comment

fix returned expressions #1644

fix returned expressions #1644

Conversation

t4c1 commented Jan 27, 2020 • edited Loading

Summary

Tests

Side Effects

Checklist

t4c1 commented Jan 27, 2020

avehtari commented Jan 27, 2020

rok-cesnovar commented Jan 27, 2020 • edited Loading

bob-carpenter commented Jan 27, 2020 • edited Loading

avehtari commented Jan 27, 2020

t4c1 commented Jan 27, 2020

bob-carpenter commented Jan 27, 2020 via email

SteveBronder commented Jan 27, 2020

rok-cesnovar commented Jan 28, 2020 • edited Loading

avehtari commented Jan 28, 2020

rok-cesnovar commented Jan 28, 2020

bob-carpenter commented Jan 28, 2020 via email

SteveBronder commented Jan 29, 2020

andrjohns Jan 29, 2020

Choose a reason for hiding this comment

t4c1 Jan 29, 2020

Choose a reason for hiding this comment

rok-cesnovar commented Jan 29, 2020

t4c1 commented Jan 29, 2020

rok-cesnovar commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

t4c1 commented Jan 29, 2020

bob-carpenter commented Jan 29, 2020 via email

rok-cesnovar commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

SteveBronder commented Jan 29, 2020

rok-cesnovar commented Jan 29, 2020

rok-cesnovar left a comment

Choose a reason for hiding this comment

t4c1 commented Jan 27, 2020 •

edited

Loading

rok-cesnovar commented Jan 27, 2020 •

edited

Loading

bob-carpenter commented Jan 27, 2020 •

edited

Loading

rok-cesnovar commented Jan 28, 2020 •

edited

Loading