Fix no instance of overloaded function `stan::math::Holder::coeffRef` #2632

hsbadr · 2021-12-23T03:36:19Z

This PR together with stan-dev/rstanarm#558 fixes building rstanarm with Stan v2.28.2.

Specifically, it fixes the following error:

R/library/StanHeaders/include/stan/math/prim/fun/scalar_seq_view.hpp(32): error: no instance of overloaded function "stan::math::Holder<ArgType, Ptrs...>::coeffRef [with ArgType=Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, Ptrs=<std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0,
          Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>>]" matches the argument list and object (the object has cv-qualifiers that prevent a match)
            argument types are: (size_t)
            object type is: const stan::ref_type_t<stan::math::Holder<Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>> &&>
    inline auto operator[](size_t i) const { return c_.coeffRef(i); }
                                                       ^
R/library/RcppEigen/include/Eigen/src/Core/ArrayBase.h(70): note: this candidate was rejected because mismatch in count of arguments
      using Base::coeffRef;
                  ^
R/library/RcppEigen/include/Eigen/src/Core/ArrayBase.h(70): note: this candidate was rejected because the selector is mismatched
      using Base::coeffRef;
                  ^
          detected during:
            instantiation of "auto stan::scalar_seq_view<C, stan::require_eigen_vector_t<C>>::operator[](size_t={unsigned long}) const [with C=stan::ref_type_t<stan::math::Holder<Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>,
                      std::remove_reference_t<std::vector<double, std::allocator<double>>>>>> &&>]" at line 87 of "R/library/StanHeaders/include/stan/math/prim/prob/gamma_lpdf.hpp"
            instantiation of "stan::return_type_t<T_y, T_shape, T_inv_scale> stan::math::gamma_lpdf<propto,T_y,T_shape,T_inv_scale,<unnamed>>(const T_y &, const T_shape &, const T_inv_scale &) [with propto=false, T_y=std::vector<stan::math::var_value<double, void>, std::allocator<stan::math::var_value<double, void>>>, T_shape=double, T_inv_scale=double, <unnamed>=(void *)nullptr]" at line 1547 of "stan_files/polr.hpp"
            instantiation of "stan::scalar_type_t<VecR> model_polr_namespace::model_polr::log_prob_impl<propto__,jacobian__,VecR,VecI,<unnamed>,<unnamed>>(VecR &, VecI &, std::ostream *) const [with propto__=false, jacobian__=false, VecR=Eigen::Matrix<stan::math::var, -1, 1, 0, -1, 1>, VecI=Eigen::Matrix<int, -1, 1, 0, -1, 1>, <unnamed>=(void *)nullptr, <unnamed>=(void *)nullptr]" at line 2116 of "stan_files/polr.hpp"
            instantiation of "T_ model_polr_namespace::model_polr::log_prob<propto__,jacobian__,T_>(Eigen::Matrix<T_, -1, 1, 0, -1, 1> &, std::ostream *) const [with propto__=false, jacobian__=false, T_=stan::math::var]" at line 96 of "R/library/StanHeaders/include/src/stan/model/model_base_crtp.hpp"
            instantiation of "stan::math::var stan::model::model_base_crtp<M>::log_prob(Eigen::Matrix<stan::math::var, -1, 1, 0, -1, 1> &, std::ostream *) const [with M=model_polr_namespace::model_polr]"

```c++ R/library/StanHeaders/include/stan/math/prim/fun/scalar_seq_view.hpp(32): error: no instance of overloaded function "stan::math::Holder<ArgType, Ptrs...>::coeffRef [with ArgType=Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, Ptrs=<std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>>]" matches the argument list and object (the object has cv-qualifiers that prevent a match) argument types are: (size_t) object type is: const stan::ref_type_t<stan::math::Holder<Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>> &&> inline auto operator[](size_t i) const { return c_.coeffRef(i); } ^ R/library/RcppEigen/include/Eigen/src/Core/ArrayBase.h(70): note: this candidate was rejected because mismatch in count of arguments using Base::coeffRef; ^ R/library/RcppEigen/include/Eigen/src/Core/ArrayBase.h(70): note: this candidate was rejected because the selector is mismatched using Base::coeffRef; ^ detected during: instantiation of "auto stan::scalar_seq_view<C, stan::require_eigen_vector_t<C>>::operator[](size_t={unsigned long}) const [with C=stan::ref_type_t<stan::math::Holder<Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>> &&>]" at line 87 of "R/library/StanHeaders/include/stan/math/prim/prob/gamma_lpdf.hpp" instantiation of "stan::return_type_t<T_y, T_shape, T_inv_scale> stan::math::gamma_lpdf<propto,T_y,T_shape,T_inv_scale,<unnamed>>(const T_y &, const T_shape &, const T_inv_scale &) [with propto=false, T_y=std::vector<stan::math::var_value<double, void>, std::allocator<stan::math::var_value<double, void>>>, T_shape=double, T_inv_scale=double, <unnamed>=(void *)nullptr]" at line 1547 of "stan_files/polr.hpp" instantiation of "stan::scalar_type_t<VecR> model_polr_namespace::model_polr::log_prob_impl<propto__,jacobian__,VecR,VecI,<unnamed>,<unnamed>>(VecR &, VecI &, std::ostream *) const [with propto__=false, jacobian__=false, VecR=Eigen::Matrix<stan::math::var, -1, 1, 0, -1, 1>, VecI=Eigen::Matrix<int, -1, 1, 0, -1, 1>, <unnamed>=(void *)nullptr, <unnamed>=(void *)nullptr]" at line 2116 of "stan_files/polr.hpp" instantiation of "T_ model_polr_namespace::model_polr::log_prob<propto__,jacobian__,T_>(Eigen::Matrix<T_, -1, 1, 0, -1, 1> &, std::ostream *) const [with propto__=false, jacobian__=false, T_=stan::math::var]" at line 96 of "R/library/StanHeaders/include/src/stan/model/model_base_crtp.hpp" instantiation of "stan::math::var stan::model::model_base_crtp<M>::log_prob(Eigen::Matrix<stan::math::var, -1, 1, 0, -1, 1> &, std::ostream *) const [with M=model_polr_namespace::model_polr]" ```

rok-cesnovar · 2021-12-23T07:31:33Z

Do we have any idea why does this pop up with rstanarm and not our unit tests in Math?

stan-buildbot · 2021-12-23T08:29:46Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.65	3.53	1.04	3.46% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.4% slower
eight_schools/eight_schools.stan	0.09	0.09	0.98	-1.6% slower
gp_regr/gp_regr.stan	0.14	0.14	1.01	1.14% faster
irt_2pl/irt_2pl.stan	5.72	5.81	0.98	-1.56% slower
performance.compilation	93.45	91.31	1.02	2.29% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.04	8.1	0.99	-0.76% slower
pkpd/one_comp_mm_elim_abs.stan	32.29	31.98	1.01	0.98% faster
sir/sir.stan	119.78	120.25	1.0	-0.39% slower
gp_regr/gen_gp_data.stan	0.03	0.04	0.99	-0.85% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.0	3.06	0.98	-2.22% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.39	0.99	-0.91% slower
arK/arK.stan	2.06	2.09	0.99	-1.49% slower
arma/arma.stan	0.23	0.23	0.99	-1.25% slower
garch/garch.stan	0.57	0.58	1.0	-0.46% slower
Mean result: 0.996260357234

Jenkins Console Log
Blue Ocean
Commit hash: 5cd98ac

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

hsbadr · 2021-12-23T09:42:09Z

Do we have any idea why does this pop up with rstanarm and not our unit tests in Math?

No sure, but either it hits a condition that isn't tested in Math (template, argument type, ... etc.) or a different version of the headers (e.g., Eigen or other dependencies) in the R environment. It isn't good for the unit tests here that the checks have passed with and without this PR. I want @SteveBronder to review it; it might be a side effect of fc7dc24 or one of the reverts after it.

It could also flag that a change is required in stanc3 (C++ code). In that case, we have to decide what we keep in Math and add a test to make sure that it fails otherwise.

rok-cesnovar · 2021-12-23T09:45:12Z

Thanks! Yeah once we figure out the details we definitely want a unit test in Math to prevent the same issue in the future.

SteveBronder · 2021-12-23T20:34:17Z

I think the issue here was just that Eigen::Holder only has non-const coeffRef() method and we are calling that from a const method. @hsbadr I made some small changes that should let everything compile

hsbadr · 2021-12-23T20:37:35Z

I think the issue here was just that Eigen::Holder only has non-const coeffRef() method and we are calling that from a const method. @hsbadr I made some small changes that should let everything compile

Thanks @SteveBronder! I'll test your changes and get back to you if I run into any related issues.

hsbadr · 2021-12-23T22:21:08Z

I made some small changes that should let everything compile

It was compiling successfully before your commit, and now it fails at line 42 with a similar error:

R/library/StanHeaders/include/stan/math/prim/fun/scalar_seq_view.hpp(42): error: no instance of overloaded function "stan::math::Holder<ArgType, Ptrs...>::coeffRef [with ArgType=Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, Ptrs=<std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0,
          Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>>]" matches the argument list and object (the object has cv-qualifiers that prevent a match)
            argument types are: (size_t)
            object type is: const stan::ref_type_t<stan::math::Holder<Eigen::ArrayWrapper<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>, std::remove_reference_t<stan::math::Holder<Eigen::Map<Eigen::Matrix<double, -1, 1, 0, -1, 1>, 0, Eigen::Stride<0, 0>>, std::remove_reference_t<std::vector<double, std::allocator<double>>>>>> &&>
      return c_.coeffRef(i);
                ^

hsbadr · 2021-12-23T23:42:25Z

@SteveBronder I've to revert your last commit to fix the new error. Feel free to make any changes, and I'll test them.

This reverts commit bf6e4fd.

stan-buildbot · 2021-12-24T02:53:25Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.57	3.66	0.97	-2.67% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.81% slower
eight_schools/eight_schools.stan	0.09	0.09	1.02	1.76% faster
gp_regr/gp_regr.stan	0.14	0.15	0.96	-3.64% slower
irt_2pl/irt_2pl.stan	5.77	5.74	1.01	0.51% faster
performance.compilation	93.46	90.94	1.03	2.7% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.18	8.14	1.0	0.44% faster
pkpd/one_comp_mm_elim_abs.stan	31.77	32.97	0.96	-3.77% slower
sir/sir.stan	121.84	119.45	1.02	1.96% faster
gp_regr/gen_gp_data.stan	0.03	0.04	0.95	-5.1% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.0	3.0	1.0	-0.05% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.4	0.98	-2.49% slower
arK/arK.stan	2.08	2.07	1.0	0.21% faster
arma/arma.stan	0.23	0.23	1.01	1.23% faster
garch/garch.stan	0.57	0.57	1.01	0.56% faster
Mean result: 0.993158579654

Jenkins Console Log
Blue Ocean
Commit hash: a69f1d9

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 · 2022-01-03T18:15:26Z

@hsbadr are you able to make a minimal stan model that shows this? I looked at the gamma_lpdf call in polr.stan and thought the below would work, but it's compiling fine on my computer with the develop version of cmdstan

data {
  real<lower=0> shape;
  real<lower=0> rate;
}
parameters {
  real<lower=0> alpha[8];
}
model {
   target += gamma_lpdf(alpha | shape, rate);
}

Are you using the develop version of Stan math? This sounds like it might be some systemic error with how we handle const in the math library I'd just like to make sure we catch this now before we end up doing a bunch of little patches here and there that get around some underlying problem.

SteveBronder

It would be nice to know the underlying issue but also I think this is good if it's delaying anything else rn

hsbadr · 2022-01-03T18:39:40Z

Are you using the develop version of Stan math?

Yes, I use the experimental branch of StanHeaders, which is at the develop version of Stan/Math.

This sounds like it might be some systemic error with how we handle const in the math library I'd just like to make sure we catch this now before we end up doing a bunch of little patches here and there that get around some underlying problem.

Agreed. I just wanted to make this visible to you as it might be an obvious typo from the previous reverts (or a bug in the C++ code generated by stanc3 for one rstanarm models). I'll debug it and add a test that captures the issue. I don't like that we pass the tests here with and without this minor patch while the latter causes a failure in building rstanarm.

SteveBronder · 2022-01-03T19:42:05Z

@hsbadr word! Does the above stan program compile fine on your machine? If so then maybe my example above is just not capturing the issue. Also if it's possible to post the full error log from the compiler as a gist or something that would be useful

https://gist.github.com/

hsbadr · 2022-01-03T20:09:43Z

Does the above stan program compile fine on your machine?

Yes, I've successfully built your Stan code above using the experimental version of rstan/StanHeaders, with this patch, and it fails without it. So. it seems that it captures the issue. Here's the error I get without this patch:

Compilation ERROR, function(s)/method(s) not created!
Error in compileCode(f, code, language = language, verbose = verbose) :
              instantiation of "stan::return_type_t<T_y, T_shape, T_inv_scale> stan::math::gamma_lpdf<propto,T_y,T_shape,T_inv_scale,<unnamed>>(const T_y &, const T_shape &, const T_inv_scale &) [with propto=false, T_y=std::vector<stan::math::var_value<double, void>, std::allocator<stan::math::var_value<double, void>>>, T_shape=double, T_inv_scale=double, <unnamed>=(void *)nullptr]" at line 129 of "file20fb3203c951.cpp"            instantiation of "stan::scalar_type_t<VecR> model20fb746d9f75__namespace::model20fb746d9f75_::log_prob_impl<propto__,jacobian__,VecR,VecI,<unnamed>,<unnamed>>(VecR &, VecI &, std::ostream *) const [with propto__=false, jacobian__=false, VecR=Eigen::Matrix<stan::math::var, -1, 1, 0, -1, 1>, VecI=Eigen::Matrix<int, -1, 1, 0, -1, 1>, <unnamed>=(void *)nullptr, <unnamed>=(void *)nullptr]" at line 322 of "file20fb3203c951.cpp"            instantiation of "T_ model20fb746d9f75__namespace::model20fb746d9f75_::log_prob<propto__,jacobian__,T_>(Eigen::Matrix<T_,

hsbadr · 2022-01-03T20:19:21Z

@SteveBronder I think it fails in the following template:

    template <bool propto__, bool jacobian__, typename T_>
    inline T_ log_prob(Eigen::Matrix<T_,Eigen::Dynamic,1>& params_r,
                       std::ostream* pstream = nullptr) const {
      Eigen::Matrix<int, -1, 1> params_i;
      return log_prob_impl<propto__, jacobian__>(params_r, params_i, pstream);
    }

Specifically, at return log_prob_impl<propto__, jacobian__>(params_r, params_i, pstream);.

hsbadr · 2022-01-03T20:32:41Z

Also if it's possible to post the full error log from the compiler as a gist or something that would be useful

Here's the C++ code. Check lines 129 and 322.

SteveBronder · 2022-01-03T21:26:34Z

Hmm interesting. So removing the Rcpp stuff this compiles with cmdstan just fine locally. Let me try to build this using experimental rstan etc. there may be something going on there. But if that's the case then I think it's fine to merge your PR for now and I can try to investigate this more later

hsbadr · 2022-01-03T21:44:39Z

I think it's fine to merge your PR for now and I can try to investigate this more later

Sounds good. If this PR has no side effects, it's good to go.

hsbadr requested a review from SteveBronder December 23, 2021 03:36

make non-const operator[] method for scalar_seq_view

bf6e4fd

SteveBronder previously approved these changes Dec 23, 2021

View reviewed changes

Revert "make non-const operator[] method for scalar_seq_view"

a69f1d9

This reverts commit bf6e4fd.

hsbadr dismissed SteveBronder’s stale review via a69f1d9 December 23, 2021 23:43

SteveBronder approved these changes Jan 3, 2022

View reviewed changes

SteveBronder merged commit 46fa82d into stan-dev:develop Jan 3, 2022

hsbadr deleted the fix/scalar_seq_view branch January 4, 2022 00:06

hsbadr mentioned this pull request Jan 5, 2022

Fix size() and namespace lookups so that we can compile with C++17 #2641

Closed

5 tasks

Uh oh!

Fix no instance of overloaded function stan::math::Holder::coeffRef #2632

Fix no instance of overloaded function stan::math::Holder::coeffRef #2632

Uh oh!

Conversation

hsbadr commented Dec 23, 2021

Uh oh!

rok-cesnovar commented Dec 23, 2021

Uh oh!

stan-buildbot commented Dec 23, 2021

Uh oh!

hsbadr commented Dec 23, 2021

Uh oh!

rok-cesnovar commented Dec 23, 2021

Uh oh!

SteveBronder commented Dec 23, 2021

Uh oh!

hsbadr commented Dec 23, 2021

Uh oh!

hsbadr commented Dec 23, 2021

Uh oh!

hsbadr commented Dec 23, 2021

Uh oh!

stan-buildbot commented Dec 24, 2021

Uh oh!

SteveBronder commented Jan 3, 2022

Uh oh!

SteveBronder left a comment

Choose a reason for hiding this comment

Uh oh!

hsbadr commented Jan 3, 2022

Uh oh!

SteveBronder commented Jan 3, 2022

Uh oh!

hsbadr commented Jan 3, 2022

Uh oh!

hsbadr commented Jan 3, 2022

Uh oh!

hsbadr commented Jan 3, 2022

Uh oh!

SteveBronder commented Jan 3, 2022

Uh oh!

hsbadr commented Jan 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix no instance of overloaded function `stan::math::Holder::coeffRef` #2632

Fix no instance of overloaded function `stan::math::Holder::coeffRef` #2632