Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace \ with function #789

Merged
merged 5 commits into from
Jul 20, 2023
Merged

replace \ with function #789

merged 5 commits into from
Jul 20, 2023

Conversation

jsocolar
Copy link
Contributor

@jsocolar jsocolar commented Jul 19, 2023

Submission Checklist

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

Summary

This enables cmdstanr to install on R versions <= 4.0. I've tested on 4.0, but not explicitly on earlier versions. Can close #788

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):
NCX, 2443 Fillmore St. #380-1418, San Francisco, CA 94115

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

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton! Will merge when/if the tests pass.

RcppEigen depends R 3.6
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2023

Codecov Report

Merging #789 (aa1545e) into master (dbf41cd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head aa1545e differs from pull request most recent head 2a002e5. Consider uploading reports for the commit 2a002e5 to get more accurate results

@@            Coverage Diff             @@
##           master     #789      +/-   ##
==========================================
- Coverage   88.24%   88.24%   -0.01%     
==========================================
  Files          12       12              
  Lines        4152     4151       -1     
==========================================
- Hits         3664     3663       -1     
  Misses        488      488              
Impacted Files Coverage Δ
R/fit.R 96.21% <100.00%> (-0.01%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jsocolar
Copy link
Contributor Author

jsocolar commented Jul 19, 2023

Looks like this is finally passing on R 3.6. Couple of things:

  • CI won't pass on R 3.5 because RcppEigen apparently depends >= 3.6. Since RcppEigen is just in suggests, maybe no change is necessary, but wanted to flag in case it's cleaner to bump the depends to R 3.6. I don't know what changed between 3.5 and 3.6, but it seems like there's always a risk of introducing patterns that break in 3.5 when we can't test 3.5 on CI.
  • R added the simplify argument to apply in 4.1. I've coded around that in 2a002e5. However, I haven't actually stepped through that function and looked at the behavior of the workaround; I'm just trusting the test harness to catch problems. Just want to flag for you that you might want to double check that change.

@rok-cesnovar
Copy link
Member

We can leave it at 3.5 for now, given that rcppeigen is not mandatory. The apply change looks fine to me, thanks for highlighting it!

@rok-cesnovar rok-cesnovar merged commit e8713b4 into stan-dev:master Jul 20, 2023
@jgabry
Copy link
Member

jgabry commented Jul 20, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmdstanr depends R >= 4.1
4 participants