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

Add flags to get_dims and get_param_names #1245

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

WardBrian
Copy link
Member

Follow on to #1241.

As discussed in stan-dev/stan#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

  • Run unit tests
  • Documentation
    • OR, no user-facing changes were made

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 WardBrian added cleanup Code simplification or clean-up cpp-codegen labels Aug 25, 2022
@WardBrian WardBrian requested a review from SteveBronder August 25, 2022 19:52
@WardBrian
Copy link
Member Author

I'm currently running this against stan-dev/stan#3139

I am also going to run stan-dev/stan#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

@WardBrian WardBrian changed the title Remove old definition of get_dims and get_param_names Add flags to get_dims and get_param_names Aug 26, 2022
@codecov
Copy link

codecov bot commented Dec 20, 2022

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%) ⬆️

@WardBrian
Copy link
Member Author

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

Merge stan-dev/stan#3139, which will fail

Merge this, wait for build to upload nightly binary

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

@WardBrian WardBrian requested a review from rok-cesnovar March 6, 2023 17:35
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.

🚀

@WardBrian WardBrian merged commit 30a6554 into master Mar 9, 2023
@WardBrian WardBrian deleted the fix/1240-remove-older-version branch March 9, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code simplification or clean-up cpp-codegen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants