[FR] Expose variadic signatures for `integrate_1d` (deprecate current?)
Opened this issue · 9 comments
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
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)
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?
Ah, yeah. That will be similar to the changes @charlesm93 and I made to the algebra solvers, which required two signatures
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
.
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)
@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?
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
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
Note: when introducing a new name, may be useful to include the algorithm in it, in anticipation of things like stan-dev/math#3000