Add get_param_names overload with flag arguments #1241
Conversation
|
Discussed in the weekly Stan meeting, @bob-carpenter confirmed it was fine to add the new arguments with defaults in model_base for backwards compatibility purposes. After this is merged we can update the model_base signatures to be virtual void get_param_names(std::vector<std::string>& names,
bool include_tparams = true,
bool include_gqs = true) const = 0; virtual void get_dims(std::vector<std::vector<size_t> >& dimss,
bool include_tparams = true,
bool include_gqs = true) const = 0; |
|
@SteveBronder this is ready for review and should be relatively quick, if you have a moment |
| (** Print the [get_param_names] method of the model class | ||
| @param ppf A pretty printer. | ||
| *) | ||
| let pp_get_param_names ppf {Program.output_vars; _} = | ||
| let add_param = fmt "%S" in | ||
| (* old -- kept as overload for compat until model_base changes *) |
There was a problem hiding this comment.
del this and the other new comment tho'
There was a problem hiding this comment.
After the change to model_base I plan on deleting the old code, and those comments
There was a problem hiding this comment.
Lets not delete the old code since we want to keep backwards compatibility (C++ sees these as two different virtual function signatures)
There was a problem hiding this comment.
We cannot expose both
virtual void get_param_names(std::vector<std::string>& names) const = 0;
and
virtual void get_param_names(std::vector<std::string>& names, const bool emit_transformed_parameters = true, const bool emit_generated_quantities = true) const = 0;
since it makes calls with only the vector argument ambiguous. Because we're providing default values for the arguments which match previous behavior, we should just keep the second
This would be the first part of #1240. After this is merged, we can safely update Stan's model_base to require these arguments, then delete the original overload, then fix the bug @SteveBronder noticed by setting the flags to false during initialization.
While we are at it, would we want to make the same change to
get_dims?Submission Checklist
Release notes
Added a new version of the model method
get_param_nameswhich allows specifiying if the transformed parameters or generated quantities should be emitted.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)