stan icon indicating copy to clipboard operation
stan copied to clipboard

Extend `get_dims` and `get_param_names` in model_base

Open WardBrian opened this issue 2 years ago • 7 comments

Submission Checklist

  • [x] Run unit tests: ./runTests.py src/test/unit
  • [x] Run cpplint: make cpplint
  • [x] Declare copyright holder and open-source license: see below

Summary

Closes https://github.com/stan-dev/stanc3/issues/1240. New flags are added to these methods to allow excluding the tparams or generated quantities, and these are used in initialization.

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

Simons Foundation

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)
  • Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

WardBrian avatar Aug 25 '22 18:08 WardBrian

It seems like we have a bit of a chicken-egg problem here. If we delete the older function definition from stanc before updating model_base, then the stanc3 tests will all fail, but if we generate both versions in stanc3, then PRs against Stan will fail due to the ambiguity of existing calls.

We reach a happy state only after model_base is updated and stanc3 generates only one signature, I believe. @SteveBronder - thoughts?

WardBrian avatar Aug 25 '22 19:08 WardBrian

Jenkins passes when using https://github.com/stan-dev/stanc3/pull/1245 up until the comparisons tests, which by definition can't run since the different versions can't use the same compiler output

WardBrian avatar Aug 31 '22 18:08 WardBrian

An @bgoodri and @hsbadr have a quick look at this and evaluate on how difficult this change will be to manage for rstan?

wds15 avatar Sep 06 '22 19:09 wds15

have a quick look at this and evaluate on how difficult this change will be to manage for rstan?

On the code side, probably nothing because of the default arguments, but at most two extra boolean arguments to get_param_names and get_dims calls.

So it's just a matter of whether it'll cause further complications with staging on CRAN for RStan and StanHeaders.

bob-carpenter avatar Sep 06 '22 20:09 bob-carpenter

All past models will have C++ code without those flags so somehow I would have to avoid passing them in that situation.

bgoodri avatar Sep 06 '22 20:09 bgoodri

OK... I was just worried about the rare need for binary compatibility in the model class, but it sounds as if we are good.

wds15 avatar Sep 06 '22 20:09 wds15

@bgoodri: Can you avoid passing them? I couldn't tell if you were saying it would be OK for RStan or a problem.

bob-carpenter avatar Sep 06 '22 22:09 bob-carpenter

Thanks to some work by @serban-nicusor-toptal this is now able to pass all the tests including the performance tests when tested against https://github.com/stan-dev/stanc3/issues/1245.

I'd love to see this merged before the next release

WardBrian avatar Mar 06 '23 17:03 WardBrian