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

[FR] Expose variadic signatures for integrate_1d (deprecate current?) #1352

Open
andrjohns opened this issue Aug 29, 2023 · 9 comments
Open
Labels
feature New feature or request good first issue Good for newcomers

Comments

@andrjohns
Copy link
Contributor

The integrate_1d implementation in the Math library supports variadic unstructured arguments, but the Stan language only exposes the theta, x_r, x_i parameterisation like the deprecated ODE signatures: https://mc-stan.org/docs/functions-reference/functions-1d-integrator.html#call-to-the-1d-integrator

I think this should be deprecated and changed to the variadic signatures for consistency with the ODE functions

@andrjohns andrjohns added the feature New feature or request label Aug 29, 2023
@WardBrian
Copy link
Member

If I'm reading the signature correctly there shouldn't be a need to deprecate the existing signature since it can just be treated as a 'variadic' signature with those 3 arguments as the 'user chosen' arguments.

We do need to actually expose that function in the math library (I don't want stanc generating code that calls an _impl function directly)

@andrjohns
Copy link
Contributor Author

An issue is going to be that the relative_tolerance argument is last and optional, so we wouldn't know if the user's last argument was a tolerance or not. We might to have separate integrate_1d and integrate_1d_tol sigs?

@WardBrian
Copy link
Member

Ah, yeah. That will be similar to the changes @charlesm93 and I made to the algebra solvers, which required two signatures

@nhuurre
Copy link
Collaborator

nhuurre commented Aug 29, 2023

the relative_tolerance argument is last and optional, so we wouldn't know if the user's last argument was a tolerance or not.

I think you can tell by comparing to the integrand function's signature. The integrand does not take relative_tolerance so if the provided variadic arguments match what the integrand is expecting there's no relative_tolerance; and if there's one argument too many, the last one is relative_tolerance.

@WardBrian
Copy link
Member

I agree that is possible, I don't think it's preferable in the actual implementation however (It would need a decent deal of special-casing, not to mention I can imagine it leading to weird error messages and/or subtle bugs if someone actually wanted a final argument they forgot to add to the function)

@andrjohns
Copy link
Contributor Author

@WardBrian So the variadic signatures would be:

real integrate_1d (function integrand, real a, real b, ...)
real integrate_1d_tol (function integrand, real a, real b, ..., real relative_tolerance)

Do those look ok?

Another little issue is that integrate_1d is currently one of the special-cases when handling the insertion of the pstream__ argument (since it's not the last argument). I can change this in the c++ pretty easily to have the pstream__ argument last, would that be preferable or would you want to add a variadic special case?

@andrjohns
Copy link
Contributor Author

An alternative would be for the _tol signature to have the tolerance argument before the function arguments:

real integrate_1d_tol (function integrand, real a, real b, real relative_tolerance, ...)

This is consistent with the variadic ODE functions but inconsistent with the previous integrate_1d signature, so I'm not 100% sure which would be best

@WardBrian
Copy link
Member

I think for an initial implementation it’s better to match previous functions in terms of both tolerance and pstream. Then adding this would be essentially free, we can just add it to the variadic table we already have. (I think we still need a new name though).

That being said, it would be nice to stop the pstream special casing eventually. Ideally we would update the C++ for every variadic function at once and then we can just rip all of that out of the compiler

@WardBrian
Copy link
Member

Note: when introducing a new name, may be useful to include the algorithm in it, in anticipation of things like stan-dev/math#3000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants