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

Compatibility fixes for cmdstan 2.33+ #843

Merged
merged 11 commits into from
Sep 13, 2023
Merged

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Sep 6, 2023

Submission Checklist

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

Summary

Fixes old array syntax in some examples.
I've been teaching a Stan workshop and noticed when showing the cmdstanr doc that we still use the old array syntax in a few places.

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):
Columbia University

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

@jgabry jgabry marked this pull request as ready for review September 6, 2023 21:41
@jgabry
Copy link
Member Author

jgabry commented Sep 6, 2023

I think there’s something else in the deprecations vignette that’s causing an error but I don’t have time to check right now. Will come back to this later tonight or in the next few days.

@jgabry
Copy link
Member Author

jgabry commented Sep 7, 2023

@rok-cesnovar @andrjohns Actually the Deprecations vignette is now just wrong because it's all about deprecation warnings but all the warnings it shows are now errors as of 2.33. So we need to decide what to do about that vignette. Do we want to rewrite it to be about fixing errors instead of warnings? Something else? Until we figure that out every cmdstanr PR will error during unit tests because this vignette won't run with 2.33

@jgabry
Copy link
Member Author

jgabry commented Sep 7, 2023

@rok-cesnovar @andrjohns Actually the Deprecations vignette is now just wrong because it's all about deprecation warnings but all the warnings it shows are now errors as of 2.33. So we need to decide what to do about that vignette. Do we want to rewrite it to be about fixing errors instead of warnings? Something else? Until we figure that out every cmdstanr PR will error during unit tests because this vignette won't run with 2.33

I pushed one commit yesterday that I thought would fix it (changing array syntax) but turns out all the other warnings are errors now too. So not sure what we want to do about that.

@rok-cesnovar
Copy link
Member

I would remove it and make an issue to make a short vignette on how to use the auto-formatter to convert these models.

@jgabry
Copy link
Member Author

jgabry commented Sep 7, 2023

That sounds good to me

@jgabry
Copy link
Member Author

jgabry commented Sep 7, 2023

This is still failing after removing the vignette, apparently because 2.33 broke a few things in cmdstanr. E.g., expose_functions seems to be broken (see #845). So I don't think I can get the tests to pass until we fix that.

@andrjohns
Copy link
Collaborator

I'm going to merge #847 into this PR/branch since the vignette deprecations are also causing the tests in that PR to fail.

Then we can make this PR a general 2.33-fixes PR if anything else pops up

@andrjohns andrjohns mentioned this pull request Sep 11, 2023
2 tasks
@andrjohns andrjohns changed the title update old array syntax in some examples Compatibility fixes for cmdstan 2.33+ Sep 11, 2023
@jgabry
Copy link
Member Author

jgabry commented Sep 11, 2023

Sounds good, thanks @andrjohns!

@jgabry jgabry mentioned this pull request Sep 11, 2023
2 tasks
@codecov-commenter
Copy link

Codecov Report

Merging #843 (869e7a8) into master (5e2551c) will decrease coverage by 1.97%.
The diff coverage is 92.30%.

❗ Current head 869e7a8 differs from pull request most recent head 1caa732. Consider uploading reports for the commit 1caa732 to get more accurate results

@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
- Coverage   88.38%   86.41%   -1.97%     
==========================================
  Files          12       12              
  Lines        4201     4212      +11     
==========================================
- Hits         3713     3640      -73     
- Misses        488      572      +84     
Files Changed Coverage Δ
R/example.R 97.56% <ø> (ø)
R/fit.R 96.22% <ø> (ø)
R/model.R 91.25% <ø> (+0.13%) ⬆️
R/utils.R 70.56% <92.30%> (-3.94%) ⬇️

... and 3 files with indirect coverage changes

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

@andrjohns
Copy link
Collaborator

@jgabry @rok-cesnovar this is passing and ready to have a look at. The only change that I'm not sure about is that reading estimates after a failed $optimize() run is now erroring when it doesn't under 2.32

No idea why, or how important it is

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.

The changes look good. I dont know about the optimize(), will investigate whether this was a change in cmdstan, but I think we can merge the PR before that.

@andrjohns andrjohns merged commit b29374e into master Sep 13, 2023
@andrjohns andrjohns deleted the fix-old-array-syntax branch September 13, 2023 09:18
@jgabry
Copy link
Member Author

jgabry commented Sep 13, 2023

Looks good to me too, thanks!

@jgabry jgabry linked an issue Sep 13, 2023 that may be closed by this pull request
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.

expose_functions errors after update to 2.33
4 participants