stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
140 stars 44 forks source link

Add flags to get_dims and get_param_names #1245

Closed WardBrian closed 1 year ago

WardBrian commented 2 years ago

Follow on to #1241.

As discussed in https://github.com/stan-dev/stan/pull/3139, updating an existing model_base method is sort of chicken-egg. This PR will fail as long as stan has the older definition of model_base, but stan will fail as long as we're generating two ambiguous definitions in the models (not all of the Stan code that needs access to the model uses model_base as the interface, it seems)

Submission Checklist

Release notes

Remove old version of get_param_names after #1241

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

WardBrian commented 2 years ago

I'm currently running this against https://github.com/stan-dev/stan/pull/3139

I am also going to run https://github.com/stan-dev/stan/pull/3139 against the binaries from this branch. If those tests all pass, I think we should merge both PRs.

If we don't want to go down this route we should revert #1241 and decide how to proceed.

@SteveBronder

codecov[bot] commented 1 year ago

Codecov Report

Merging #1245 (bbbc93d) into master (d61d56c) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
+ Coverage   88.95%   88.98%   +0.02%     
==========================================
  Files          64       64              
  Lines        9710     9734      +24     
==========================================
+ Hits         8638     8662      +24     
  Misses       1072     1072              
Impacted Files Coverage Δ
src/stan_math_backend/Lower_program.ml 99.06% <100.00%> (+0.07%) :arrow_up:
WardBrian commented 1 year ago

This and https://github.com/stan-dev/stan/pull/3139 are now passing against each other. The merge order will need to be:

Merge https://github.com/stan-dev/stan/pull/3139, which will fail

Merge this, wait for build to upload nightly binary

Re-run stan-dev/stan develop branch, will pass