stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
138 stars 44 forks source link

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

Open andrjohns opened 10 months ago

andrjohns commented 10 months ago

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

WardBrian commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

nhuurre commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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