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.

Sign up for GitHub

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

Jump to bottom

Expose variadic arguments for algebra_solver #2818

Closed
charlesm93 opened this issue Oct 1, 2022 · 5 comments · Fixed by #2820
Closed

Expose variadic arguments for algebra_solver #2818

charlesm93 opened this issue Oct 1, 2022 · 5 comments · Fixed by #2820
Assignees
@charlesm93
Labels
feature

Comments

@charlesm93
Copy link
Contributor

Description

The algebraic solvers, algebra_solver_newton and algebra_solver_powell aren't using variadic arguments. Most of the code seems to be already there but it is all wrapped in a function that still uses the strict function signature. We need to create the following updated signatures:

  • algebra_solver_newton(f, x0, ...)
  • algebra_solver_tol_newton(f, x0, scaling_step_size, function_tolerance, max_num_steps, ...)
  • algebra_solver_powell(f, x0, ...)
  • algebra_solver_tol_powell(f, x0, relative_tolerance, function_tolerance, max_num_steps, ...)

Open question: should we keep the relative_tolerance tuning parameter used for Powell's method? Right now, the two solvers have slightly different choices of tuning parameters, which has to do with the underlying algorithms they use. I think that's fine but it might be confusing to the user.

Current Version:

v4.4.0
@charlesm93 charlesm93 self-assigned this Oct 1, 2022
@charlesm93 charlesm93 mentioned this issue Oct 1, 2022
Closed
@WardBrian
Copy link
Member

The variadic interface would completely subsume the original, correct? I only ask because we currently have a technical restriction in Stanc where a variadic function cannot have non-variadic overloads

@charlesm93
Copy link
Contributor Author

charlesm93 commented Oct 2, 2022 •
edited
Loading

My plan is to only expose variadic arguments for algebra_solver_powell and algebra_solver_newton (as well as algebra_solver_powell_tol and algebra_solver_newton_tol) but keep the non-variadic algebra_solver for backward compatibility. The latter is a function we intend to deprecate, so I guess we'd need a warning message to that effect when compiling the Stan program.

But no, no overloading for variadic functions.

@WardBrian
Copy link
Member

I don’t think my question was clear, we currently already have an exposed function called algebra_solver _newton with argumentsfunction algebra_system, vector y_guess, vector theta, data array[] real x_r, array[] int x_i - this is strictly a subset of the signatures a variadic version of this would allow, right? So any pre-existing usage of this function should be able to become the variadic version without any changes to the model

@charlesm93
Copy link
Contributor Author

Thanks for clarifying. Yes, you're right. We actually won't break backwards compatibility. I got a little bit mixed up: in stan-math we do break backwards compatibility because we have to reposition the std::ostream* const msgs argument. But I now realize this "backwards breaking" won't propagate to the Stan language.

@charlesm93 charlesm93 mentioned this issue Oct 3, 2022
Merged
10 tasks
@charlesm93
Copy link
Contributor Author

stan-math PR is in. Once this gets reviewed and merged, we can handle the Stan language. And then will properly clean up the doc.

@WardBrian WardBrian linked a pull request Oct 31, 2022 that will close this issue
Feature/issue 2828 algebra solver variadic #2820
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charlesm93 charlesm93

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature/issue 2828 algebra solver variadic stan-dev/math
2 participants
@charlesm93 @WardBrian