csr_matrix_times_vector2 now uses csr_matrix_times_vector #514

SteveBronder · 2021-05-07T17:37:47Z

Following the conversation on a stanc3 pr this modifies csr_times_matrix_vector2 to have the same signature as the new stanc3 generated signature. It also just uses csr_matrix_times_vector since in the newest Stan math there is a reverse mode specialization for the csr_matrix_times_vector function.

jgabry · 2021-05-07T18:05:40Z

Thanks Steve! Will let @bgoodri review this since it's more in his wheelhouse

hsbadr

@SteveBronder Is this backward compatible with the current version of StanHeaders/rstan on CRAN? If not, we can branch the code based on USE_STANC3 macro.

hsbadr · 2021-05-07T18:29:28Z

inst/include/csr_matrix_times_vector2.hpp

+ template <typename T2__, typename T5__>
+ Eigen::Matrix<stan::promote_args_t<stan::value_type_t<T2__>,
+ stan::value_type_t<T5__>>, -1, 1>
+ csr_matrix_times_vector2(const int& m, const int& n, const T2__& w_arg__,
+                          const std::vector<int>& v,
+                          const std::vector<int>& u, const T5__& b_arg__,
+                          std::ostream* pstream__) {
+  return stan::math::csr_matrix_times_vector(m, n, w_arg__, v, u, b_arg__);


Also, this is related to a specific version of stanc3 > v2.26.1, after stan-dev/stanc3#865.

Ah I did exactly this but idt we need the macro branch? I think the compiler should be able to figure out which version to use

I think this is resolved now:

* checking whether package ‘rstanarm’ can be installed ... OK

I'll re-check with the development version of rstan, after it's merged.

hsbadr · 2021-05-07T19:20:13Z

Following the conversation on a stanc3 pr this modifies csr_times_matrix_vector2 to have the same signature as the new stanc3 generated signature. It also just uses csr_matrix_times_vector since in the newest Stan math there is a reverse mode specialization for the csr_matrix_times_vector function.

@SteveBronder I confirm that this patch fixes the following error:

** testing if installed package can be loaded from temporary location
Error: package or namespace load failed for ‘rstanarm’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object 'rstanarm.so':
  rstanarm.so: undefined symbol: _ZN24model_binomial_namespace24csr_matrix_times_vector2IN5Eigen3MapINS1_6MatrixIdLin1ELi1ELi0ELin1ELi1EEELi0ENS1_6StrideILi0ELi0EEEEES4_EENS3_IN5boost4math5tools12promote_argsIN4stan10value_typeIT_vE4typeENSD_IT0_vE4typeEffffE4typeELin1ELi1ELi0ELin1ELi1EEERKiSO_RKSE_RKSt6vectorIiSaIiEESV_RKSH_PSo
Error: loading failed

SteveBronder · 2021-05-07T19:26:32Z

Yeah it looks like the failure is something about the USELTO field in the description. I think the github action needs updated to a newer version of R

jgabry · 2021-05-07T19:42:12Z

I think the github action needs updated to a newer version of R

It's already using the latest release and development versions:

rstanarm/.github/workflows/R-CMD-check.yaml

Lines 25 to 26 in ef08f58

    
           - {os: ubuntu-20.04, r: 'release', rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest"} 
        
           - {os: ubuntu-20.04, r: 'devel', rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest"}

but it does seem to be complaining about the UseLTO in the DESCRIPTION file. @bgoodri Any idea why R cmd check (via GitHub actions) says

Unknown, possibly mis-spelled, fields in DESCRIPTION:
  ‘UseLTO’

even though it's running the latest R?

It's also entirely possible that the R cmd check failure has nothing to do with this and it's something else that's causing it to fail (I can't quite tell from the workflow logs if the UseLTO issue is an Error or a Note)

bgoodri · 2021-05-07T19:49:25Z

Might need R devel or 4.1 when it comes out, but that isn't an error.

…

On Fri, May 7, 2021 at 3:42 PM Jonah Gabry ***@***.***> wrote: I think the github action needs updated to a newer version of R It's already using the latest release and development versions: https://github.com/stan-dev/rstanarm/blob/ef08f58b1caeecff1a96e1e595f6741798ebbe5f/.github/workflows/R-CMD-check.yaml#L25-L26 but it does seem to be complaining about the UseLTO in the DESCRIPTION file. @bgoodri <https://github.com/bgoodri> Any idea why R cmd check (via GitHub actions) says Unknown, possibly mis-spelled, fields in DESCRIPTION: ‘UseLTO’ even though it's running the latest R? It's also entirely possible that the R cmd check failure has nothing to do with this and it's something else that's causing it to fail (I can't quite tell from the workflow logs if the UseLTO issue is an Error or a Note) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#514 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ2XKT532IGUWMALWODO73TMQ7CRANCNFSM44KUVW7A> .

hsbadr · 2021-05-07T19:55:47Z

Yeah it looks like the failure is something about the USELTO field in the description. I think the github action needs updated to a newer version of R

It's just a NOTE, not error, about UseLTO. I think it fails here:

Wrote C++ file "stan_files/polr.cc"
g++ -std=gnu++14 -I"/opt/R/4.0.5/lib/R/include" -DNDEBUG -I"../inst/include" -I"/home/runner/work/_temp/Library/StanHeaders/include/src" -DBOOST_DISABLE_ASSERTS -DEIGEN_NO_DEBUG -I'/home/runner/work/_temp/Library/StanHeaders/include' -I'/home/runner/work/_temp/Library/rstan/include' -I'/home/runner/work/_temp/Library/BH/include' -I'/home/runner/work/_temp/Library/Rcpp/include' -I'/home/runner/work/_temp/Library/RcppEigen/include' -I'/home/runner/work/_temp/Library/RcppParallel/include' -I/usr/local/include  `"/opt/R/4.0.5/lib/R/bin/Rscript" -e "RcppParallel::CxxFlags()"` `"/opt/R/4.0.5/lib/R/bin/Rscript" -e "StanHeaders:::CxxFlags()"` -fpic  -g -O2  -c init.cpp -o init.o
Wrote C++ file "stan_files/continuous.cc"
Error in readRDS("/tmp/RtmpJsRlRF/file755878ce8f61") : 
  error reading from connection
Calls: .Last -> readRDS
Execution halted
make: *** [Makevars:23: stan_files/continuous.cc] Error 1
make: *** Waiting for unfinished jobs....

@jgabry Where did it call readRDS()? I haven't looked at the code yet.

jgabry · 2021-05-07T20:07:21Z

Might need R devel or 4.1 when it comes out, but that isn't an error.
…

It has the same Note about UseLTO with R-devel (we have GitHub actions checking both release and devel). So maybe it needs 4.1. But @bgoodri if it needs 4.1 then how did you get UseLTO to work when you added it? Or am I misunderstanding something?

jgabry · 2021-05-07T20:14:04Z

@hsbadr thanks for helping out with this!

@jgabry Where did it call readRDS()? I haven't looked at the code yet.

I'm not sure where, we don't call readRDS() in rstanarm itself but maybe R cmd check does?

Where are you finding these lines with the error? I tried searching for "readRDS" in the GitHub actions logs and it doesn't come up with any results.

hsbadr · 2021-05-07T20:16:50Z

@jgabry Since it doesn't fail locally, could you try to clear the R package cache by changing the number in the following lines:

rstanarm/.github/workflows/R-CMD-check.yaml

Lines 60 to 61 in ef08f58

    
           key: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1-${{ hashFiles('.github/depends.Rds') }} 
        
           restore-keys: ${{ runner.os }}-${{ hashFiles('.github/R-version') }}-1-

You may try to replace -1- with -2- to build a new cache. The cached packages might need to be rebuilt after R-devel 4.2.

Where are you finding these lines with the error? I tried searching for "readRDS" in the GitHub actions logs and it doesn't come up with any results.

From the logs of the previous GHA.

@hsbadr

clear package cache as suggested by @hsbadr in #514 (comment)

jgabry · 2021-05-07T20:25:21Z

Thanks. I just changed the 1s to 2s in a6a8e6b (let me know if I misunderstood your suggestion). Let's see what happens.

SteveBronder · 2021-05-07T20:27:53Z

Okay so now I think I'm seeing a real error and I have a fix for it

hsbadr · 2021-05-07T20:29:08Z

Thanks. I just changed the 1s to 2s in a6a8e6b (let me know if I misunderstood your suggestion). Let's see what happens.

Thanks! That's exactly it. If it fails due to the cached packages, that will fix it. Otherwise, we may find the error in the new logs.

hsbadr · 2021-05-07T21:07:45Z

@jgabry There's 2 test failures, in test_pp_check.R. That means the build is successful, but the tests aren't.

Error: Running examples in ‘rstanarm-Ex.R’ failed
Error: Error in eval(predvars, data, env) : object 'logBili' not found
Calls: posterior_traj ... model.offset -> model.frame -> model.frame.default -> eval -> eval
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
    5. ├─base::suppressWarnings(...)
    6. │ └─base::withCallingHandlers(...)
    7. ├─bayesplot::pp_check(...)
    8. └─rstanarm:::pp_check.stanreg(...)
    9.   ├─rstanarm:::.ppc_y_and_yrep(...)
   10.   │ ├─rstantools::posterior_predict(...)
   11.   │ └─rstanarm:::posterior_predict.stanreg(...)
   12.   │   ├─rstanarm:::pp_args(...)
   13.   │   └─rstanarm:::pp_eta(object, dat, draws, m = m)
   14.   └─rstanarm:::.set_nreps(nreps, fun = plotfun_name)
  
  [ FAIL 2 | WARN 282 | SKIP 2 | PASS 2695 ]
  Error: Test failures

Also, there's a few convergence warnings that could be related:

Warning: Markov chains did not converge! Do not analyze results!

It seems that you may need to update the examples in the tests to make sure they converge, which may be the culprit for object 'logBili' not found error.

The traceback points at bayesplot::pp_check() outside rstanarm, which may have a bug or needs an update.

hsbadr · 2021-05-07T21:23:13Z

@jgabry Here's the raw logs for the failed tests:

 ══ Failed tests ════════════════════════════════════════════════════════════════
 ── Error (test_pp_check.R:56:5): pp_check.stanreg creates ggplot object ────────
 ##[error]Error: Plotting function not supported. (If the plotting function is included in the output from bayesplot::available_ppc() then it should be available via pp_check and this error is probably a bug.)
 Backtrace:
      █
   1. ├─global::expect_gg(...) test_pp_check.R:56:4
   2. │ └─testthat::expect_is(x, "ggplot", info = info, label = label)
   3. │   └─testthat::quasi_label(enquo(object), label, arg = "object")
   4. │     └─rlang::eval_bare(expr, quo_get_env(quo))
   5. ├─base::suppressWarnings(pp_check(fit, plotfun = f, nreps = j))
   6. │ └─base::withCallingHandlers(...)
   7. ├─bayesplot::pp_check(fit, plotfun = f, nreps = j)
   8. └─rstanarm:::pp_check.stanreg(fit, plotfun = f, nreps = j)
   9.   ├─rstanarm:::.ppc_y_and_yrep(...)
  10.   │ ├─rstantools::posterior_predict(...)
  11.   │ └─rstanarm:::posterior_predict.stanreg(...)
  12.   │   ├─rstanarm:::pp_args(...)
  13.   │   └─rstanarm:::pp_eta(object, dat, draws, m = m)
  14.   └─rstanarm:::.set_nreps(nreps, fun = plotfun_name)

 ── Error (test_pp_check.R:64:5): pp_check.stanreg creates ggplot object for grouped functions ──
 ##[error]Error: Plotting function not supported. (If the plotting function is included in the output from bayesplot::available_ppc() then it should be available via pp_check and this error is probably a bug.)
 Backtrace:
      █
   1. ├─global::expect_gg(...) test_pp_check.R:64:4
   2. │ └─testthat::expect_is(x, "ggplot", info = info, label = label)
   3. │   └─testthat::quasi_label(enquo(object), label, arg = "object")
   4. │     └─rlang::eval_bare(expr, quo_get_env(quo))
   5. ├─base::suppressWarnings(...)
   6. │ └─base::withCallingHandlers(...)
   7. ├─bayesplot::pp_check(...)
   8. └─rstanarm:::pp_check.stanreg(...)
   9.   ├─rstanarm:::.ppc_y_and_yrep(...)
  10.   │ ├─rstantools::posterior_predict(...)
  11.   │ └─rstanarm:::posterior_predict.stanreg(...)
  12.   │   ├─rstanarm:::pp_args(...)
  13.   │   └─rstanarm:::pp_eta(object, dat, draws, m = m)
  14.   └─rstanarm:::.set_nreps(nreps, fun = plotfun_name)
 
 [ FAIL 2 | WARN 282 | SKIP 2 | PASS 2695 ]

jgabry · 2021-05-07T23:17:21Z

Thanks a lot @hsbadr!

I just fixed the pp_check test on master (I had added a new plotting function in bayesplot that rstanarm didn't know how to handle but the test was set up to try all plotting functions) and I merged the fix into this PR. So now the only failure should be the logBili thing and you can ignore that for now (we know why it happens, just haven't finished fixing it yet). So if this PR passes everything except for the logBili error we can consider that a pass.

jgabry · 2021-05-07T23:18:22Z

Okay so now I think I'm seeing a real error and I have a fix for it

@SteveBronder Do you mean in one of the tests or when it was trying to build/install?

hsbadr · 2021-05-07T23:23:57Z

I just fixed the pp_check test on master

Thank you for the quick fix!

So if this PR passes everything except for the logBili error we can consider that a pass.

Yes, I think if it fails on tests after successful build, this PR is ready to go. If not, we know it's due to backward compatibility and should be fixed as suggested above. I've already tested this PR locally with the development versions of Math/Stan and the nightly stanc3.

jgabry · 2021-05-07T23:29:42Z

I've already tested this PR locally with the development versions of Math/Stan and the nightly stanc3.

Awesome, thank you!

SteveBronder · 2021-05-07T23:51:19Z

Nvm what I said thank ypu v much @jgabry and @hsbadr

hsbadr · 2021-05-07T23:58:29Z

Nvm what I said thank ypu v much @jgabry and @hsbadr

As expected, this change isn't backward compatible:

##[error]Error: package or namespace load failed for ‘rstanarm’ in dyn.load(file, DLLpath = DLLpath, ...):
unable to load shared object '/home/runner/work/rstanarm/rstanarm/check/rstanarm.Rcheck/00LOCK-rstanarm/00new/rstanarm/libs/rstanarm.so':
/home/runner/work/rstanarm/rstanarm/check/rstanarm.Rcheck/00LOCK-rstanarm/00new/rstanarm/libs/rstanarm.so:
  undefined symbol: _ZN21model_count_namespace24csr_matrix_times_vector2IddEEN5Eigen6MatrixIN5boost4math5tools12promote_argsIT_T0_ffffE4typeELin1ELi1ELi0ELin1ELi1EEERKiSD_RKNS2_IS7_Lin1ELi1ELi0ELin1ELi1EEERKSt6vectorIiSaIiEESL_RKNS2_IS8_Lin1ELi1ELi0ELin1ELi1EEEPSo
##[error]Error: loading failed

You may use my suggestion above, assuming that stanc3 > 2.26.1 isn't required or somehow check stanc3 version as well.

PS: You can parse the stanc3 version from system.file("stanc.js", package = "rstan") if it exists and define USE_STANC3 accordingly. If it doesn't, then use the old code.

SteveBronder · 2021-05-08T00:17:19Z

I'm afk but I can have a fix that should makes this backwards compatible

hsbadr · 2021-05-08T00:19:42Z

I'm afk but I can have a fix that should makes this backwards compatible

Sounds good. I've just mentioned that, in general, if system.file("stanc.js", package = "rstan") exists, we can parse stanc3 version from it and define USE_STANC3.

hsbadr · 2021-05-08T01:18:46Z

inst/include/csr_matrix_times_vector2.hpp

+ template <typename T2__, typename T5__>
+ Eigen::Matrix<stan::promote_args_t<stan::value_type_t<T2__>,
+ stan::value_type_t<T5__>>, -1, 1>
+ csr_matrix_times_vector2(const int& m, const int& n, const T2__& w_arg__,
+                          const std::vector<int>& v,
+                          const std::vector<int>& u, const T5__& b_arg__,
+                          std::ostream* pstream__) {
+  return stan::math::csr_matrix_times_vector(m, n, w_arg__, v, u, b_arg__);


I think this is resolved now:

* checking whether package ‘rstanarm’ can be installed ... OK

I'll re-check with the development version of rstan, after it's merged.

hsbadr · 2021-05-08T02:00:40Z

@SteveBronder @jgabry LGTM. It's successfully built with both the CRAN and dev versions of rstan (Stan/Math/stanc3).

…m/SteveBronder/rstanarm into pr/514

jgabry · 2021-05-08T16:45:40Z

@SteveBronder @jgabry LGTM. It's successfully built with both the CRAN and dev versions of rstan (Stan/Math/stanc3).

Awesome, thanks for checking!

hsbadr · 2021-06-20T22:45:35Z

@bgoodri @jgabry Is there any reason why this isn't merged yet? We need it to upgrade RStan to 2.27.

bgoodri · 2021-06-21T05:50:11Z

If it is going to call stan::math::csr_matrix_times_vector, then we might as well just change the Stan code to do that. The only reason we had csr_matrix_times_vector2 was to avoid the slowdown from repeatedly checking constants, but I presume those checks are still happening.

…

On Sun, Jun 20, 2021 at 6:45 PM Hamada S. Badr ***@***.***> wrote: @bgoodri <https://github.com/bgoodri> @jgabry <https://github.com/jgabry> Is there any reason why this isn't merged yet? We need it to upgrade RStan to 2.27. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#514 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ2XKXGD3Q2AAUNZF4C2X3TTZVRXANCNFSM44KUVW7A> .

hsbadr · 2021-12-23T01:19:03Z

If it is going to call stan::math::csr_matrix_times_vector, then we might as well just change the Stan code to do that.

Agreed. I'd prefer to change it in the Stan code and completely remove csr_matrix_times_vector2.

csr_matrix_times_vector2 now uses csr_matrix_times_vector

e3fb1f0

SteveBronder mentioned this pull request May 7, 2021

Use Eigen Maps for data and transformed data stan-dev/stanc3#865

Merged

jgabry requested a review from bgoodri May 7, 2021 18:05

hsbadr reviewed May 7, 2021

View reviewed changes

jgabry added a commit that referenced this pull request May 7, 2021

Update R-CMD-check.yaml

a6a8e6b

clear package cache as suggested by @hsbadr in #514 (comment)

Merge branch 'master' into pr/514

55981d9

also include the old version for backwards compatability

e0fb4fc

hsbadr approved these changes May 8, 2021

View reviewed changes

jgabry added 2 commits May 8, 2021 10:44

Merge branch 'master' into pr/514

674e9f7

Merge branch 'fix/csr-matrix-times-vector2-orig' of https://github.co…

01ae484

…m/SteveBronder/rstanarm into pr/514

Merge branch 'master' into pr/514

6e3e23c

hsbadr mentioned this pull request Jun 20, 2021

Merge Math release v4.0.1 into StanHeaders_2.26 stan-dev/math#2380

Merged

hsbadr mentioned this pull request Dec 23, 2021

Remove custom C++ functions #558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csr_matrix_times_vector2 now uses csr_matrix_times_vector #514

csr_matrix_times_vector2 now uses csr_matrix_times_vector #514

SteveBronder commented May 7, 2021

jgabry commented May 7, 2021

hsbadr left a comment

This comment was marked as resolved.

hsbadr May 7, 2021

SteveBronder May 8, 2021

hsbadr May 8, 2021

hsbadr commented May 7, 2021

SteveBronder commented May 7, 2021

jgabry commented May 7, 2021

bgoodri commented May 7, 2021 via email

hsbadr commented May 7, 2021

jgabry commented May 7, 2021 •

edited

Loading

jgabry commented May 7, 2021

hsbadr commented May 7, 2021

jgabry commented May 7, 2021

SteveBronder commented May 7, 2021

hsbadr commented May 7, 2021

hsbadr commented May 7, 2021 •

edited

Loading

hsbadr commented May 7, 2021

jgabry commented May 7, 2021 •

edited

Loading

jgabry commented May 7, 2021

hsbadr commented May 7, 2021

jgabry commented May 7, 2021

SteveBronder commented May 7, 2021

hsbadr commented May 7, 2021 •

edited

Loading

SteveBronder commented May 8, 2021

hsbadr commented May 8, 2021

hsbadr May 8, 2021

hsbadr commented May 8, 2021

jgabry commented May 8, 2021

hsbadr commented Jun 20, 2021

bgoodri commented Jun 21, 2021 via email

hsbadr commented Dec 23, 2021

csr_matrix_times_vector2 now uses csr_matrix_times_vector #514

Are you sure you want to change the base?

csr_matrix_times_vector2 now uses csr_matrix_times_vector #514

Conversation

SteveBronder commented May 7, 2021

jgabry commented May 7, 2021

hsbadr left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

hsbadr May 7, 2021

Choose a reason for hiding this comment

SteveBronder May 8, 2021

Choose a reason for hiding this comment

hsbadr May 8, 2021

Choose a reason for hiding this comment

hsbadr commented May 7, 2021

SteveBronder commented May 7, 2021

jgabry commented May 7, 2021

bgoodri commented May 7, 2021 via email

hsbadr commented May 7, 2021

jgabry commented May 7, 2021 • edited Loading

jgabry commented May 7, 2021

hsbadr commented May 7, 2021

jgabry commented May 7, 2021

SteveBronder commented May 7, 2021

hsbadr commented May 7, 2021

hsbadr commented May 7, 2021 • edited Loading

hsbadr commented May 7, 2021

jgabry commented May 7, 2021 • edited Loading

jgabry commented May 7, 2021

hsbadr commented May 7, 2021

jgabry commented May 7, 2021

SteveBronder commented May 7, 2021

hsbadr commented May 7, 2021 • edited Loading

SteveBronder commented May 8, 2021

hsbadr commented May 8, 2021

hsbadr May 8, 2021

Choose a reason for hiding this comment

hsbadr commented May 8, 2021

jgabry commented May 8, 2021

hsbadr commented Jun 20, 2021

bgoodri commented Jun 21, 2021 via email

hsbadr commented Dec 23, 2021

jgabry commented May 7, 2021 •

edited

Loading

hsbadr commented May 7, 2021 •

edited

Loading

jgabry commented May 7, 2021 •

edited

Loading

hsbadr commented May 7, 2021 •

edited

Loading