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

Extend get_dims and get_param_names in model_base #3139

Merged
merged 10 commits into from
Mar 9, 2023

Conversation

WardBrian
Copy link
Member

Submission Checklist

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

Summary

Closes stan-dev/stanc3#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:

@WardBrian WardBrian requested a review from SteveBronder August 25, 2022 18:08
@WardBrian
Copy link
Member Author

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 WardBrian marked this pull request as draft August 26, 2022 13:28
@WardBrian WardBrian marked this pull request as ready for review August 31, 2022 18:54
@WardBrian
Copy link
Member Author

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

Copy link
Contributor

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

This looks good on the C++ side. Thanks for adding docs and tests.

I'll defer to others more in touch with our CI to merge.

@wds15
Copy link
Contributor

wds15 commented Sep 6, 2022

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

@bob-carpenter
Copy link
Contributor

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.

@bgoodri
Copy link
Contributor

bgoodri commented Sep 6, 2022 via email

@wds15
Copy link
Contributor

wds15 commented Sep 6, 2022

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

@bob-carpenter
Copy link
Contributor

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

@WardBrian
Copy link
Member Author

WardBrian commented Mar 6, 2023

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

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

@WardBrian WardBrian merged commit 5118264 into develop Mar 9, 2023
@WardBrian WardBrian deleted the feature/param-names-dims-flags branch March 9, 2023 20:05
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.

[FR] get_param_names should be able to only return object names in parameters block
5 participants