Fix error with off by one indexing

[johnlees]

Submission Checklist

• Run unit tests
• Declare copyright holder and agree to license (see below)

Summary

There appears to be an off-by-one indexing issue in R vs C++. The default index of 0 found in the metadata is invalid for R lists which are 1-indexed. This leads to the following error, which is difficult for the user to comprehend:

read_cmdstan_csv('cmdstan.run0.csv')
Error in inv_metric[[metadata$id]] <- metadata$inv_metric :
  attempt to select less than one element in integerOneIndex

The right solution to me would seem to be to convert the index into R's 1-index form, where they are used in this manner.

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
John Lees

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)

[johnlees]

There are some checks on the length of the MCMC failing. I'm not sure what the best way to resolve these are without just hacking them. The files included in tests/testthat/resources/csv/ all have correct indices for R's 1-based indexing, so perhaps this is only a problem when running cmdstan from the command line?

I think this is worth looking at, because running cmdstan as in the manual, followed by rcmdstan gives this error. Perhaps adding an 'index_adjust' option to read_cmdstan_csv() might be better?

[jgabry]

Thanks for reporting this. I just opened an issue (#292) about this and I'll respond more over there (so we can have the discussion in a separate issue in case this particular PR doesn't end up being the solution).

[rok-cesnovar]

@johnlees was there an issue if you dealt with it as a character? If some tests failed, those tests might need a revisit?

[johnlees]

Yes, sorry I was getting:

  ── 2. Error: read_cmdstan_csv() reads adaptation step size correctly (@test-csv.
  subscript out of bounds
  Backtrace:
   1. testthat::expect_equal(csv_out$step_size[[2]], 0.672434)
   2. testthat::quasi_label(enquo(object), label, arg = "object")
   3. rlang::eval_bare(expr, quo_get_env(quo))
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 868 | SKIPPED: 3 | WARNINGS: 2 | FAILED: 2 ]
  1. Error: read_cmdstan_csv() returns correct diagonal of inverse mass matrix (@test-csv.R#165) 
  2. Error: read_cmdstan_csv() reads adaptation step size correctly (@test-csv.R#376) 

I think changing to csv_out$step_size[[as.character(2)]] would work. I will try that! (sorry for the CI use, I don't have a good setup available to me to run the tests locally)

[johnlees]

@rok-cesnovar An error in the test coverage, but otherwise passes. Does this fix look like what you envisioned?

[rok-cesnovar]

sorry for the CI use, I don't have a good setup available to me to run the tests locally

No problem at all! Its free Github instances :)

Thanks for working on this!

[rok-cesnovar]

Does this fix look like what you envisioned?

I think so yes. Will take a look shortly.

[jgabry]

It looks like the test coverage failure is due to the tests for install_cmdstan(), which is unrelated to this PR. @rok-cesnovar maybe this is because of the 2.24.1 patch release?

[rok-cesnovar]

Argh, yes. https://github.com/stan-dev/cmdstanr/blob/master/tests/testthat/helper-envvars-and-paths.R#L18 needs to be updated. Want to push to this PR or master @jgabry? I wont be at a computer till tomorrow morning. Can do it then.

[jgabry]

Yeah, no problem, I can fix that now.

[codecov-commenter]

Codecov Report

Merging #291 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #291   +/-   ##
=======================================
  Coverage   90.11%   90.11%           
=======================================
  Files          12       12           
  Lines        2419     2419           
=======================================
  Hits         2180     2180           
  Misses        239      239           

Impacted Files	Coverage Δ
R/read_csv.R	96.28% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35c7668...cdeccde. Read the comment docs.

[jgabry]

@johnlees Looks good, thanks! I'm approving but will wait to merge in case @rok-cesnovar notices anything I missed.

[rok-cesnovar]

Thanks @johnlees!