Don't rename functions or file basenames without good reason

Yes. By meta-header, I mean things like math.hpp that only #include other files, in which case renaming the files #included is not that big a deal.

cholesky_corr_... functions were renamed to cholesky_factor_corr_... because there were inconsistencies in the parser names and functions, which were perhaps historical - were the cholesky factor covariance functions added after the cholesky factor correlation functions?

the renaming was intended to make the compiler code cleaner across the parser and generator.

agreed that it’s a problem if the interfaces know too much about the internals of the compiler - I would argue that they shouldn’t - because otherwise, we’ll never be able to pay down technical debt which is what the parser/generator refactor that went in for 2.19 was intended to do.

Should we add something about this to the PR template?

I realize cholesky_factor_... is a bit more explicit, but that only helps developers. I doubt it would often be worth it to break existing C++ code. What I have had to do is put the old functions back into StanHeaders and just have them call the new functions, which could have been done upstream until Stan3.

sorry to have caused problems here. had I realized, I would have let this one go.

Is the problem that other R packages have started using this and they now break? Yack!!

In case this is really related to other R packages giving you problems due to files being moved around… does this interact with the flattening refactor?

No, I think the flattening refactor will be mostly okay due to meta-includes. Maybe a couple of packages that link to StanHeaders but don’t build Stan models will be affected.

Don’t rename functions or file basenames without good reason

Including individual files is out of scope for math lib API support as laid out by @seantalts (see point 1).

I believe point 2 in the supported API doc lays out that no functions will ever change name or argument structure within those meta-headers. So I think changing the name of a function violated that rule.

Going forward, RStan’s going to need to stop including files on an ad hoc basis and get what it needs included in the bigger headers.

P.S. There’s already a huge Discourse topic on what should be supported..

1 Like

the change predates the rule, but is a good illustration of why it’s there.

the change was made because the generator had a bunch of special cases just for cholesky factor correlation variables because the math lib function names didn’t line up with the type names - at least I’m pretty sure that’s why I went so far as to change the function names. so, trying to pay off technical debt just borrows from Rstan to pay core Stan or something like that.

1 Like

We also haven’t defined an API for the Stan compiler yet. *ducks*

This is me being silly and pedantic, but I think it’s a good time to point out that e.g. @bgoodri doing a bunch of work now to try to find the new .hpp files and function names is going to be wasted effort once stanc3 is out relatively soon. The safest interface to the Stan compiler is, to my reckoning, as an executable that transforms Stan into a C++ model with a defined interface. We’ll probably also add a function to dump Stan lib type signatures, and happily whatever other functions @bgoodri wants to support a fancy and robust RStan. But we need to be relying on specific supported endpoints upstream, not arbitrary compiler source code. So I think @mitzimorris was totally within her rights here to refactor the compiler as she saw fit given the lack of such a guarantee and the near future of a non-C++ compiler.

[edit] P.S. @bgoodri if you have such a wish-list it’s a good time to send it to us so we can get start integrating it :)

My bad. I thought we were talking about a function in the math lib.

@bgoodri — the transforms are available in the math lib. The cholesky_corr_unconstrain is a method in the class stan::io::writer.

@Matthijs or @seantalts — is the current plan to generate the transform code and get rid of this writer class in C++? I don’t know how much manual unfolding you want to do compared to what’s going on now.

Yeah, I’m planning to move more and more of that logic into the compiler itself, though I’m also being pragmatic. In this specific case, I’m planning to bypass the reader and writer class completely and use the underlying Math lib functions for constraining and freeing.

Ben fyi I just merged in a PR in math that deleted

stan/math/fwd/scal/meta/ad_promotable.hpp

stan/math/rev/scal/meta/ad_promotable.hpp

stan/math/prim/arr/meta/contains_std_vector.hpp

stan/math/prim/mat/meta/is_vector.hpp

stan/math/prim/mat/meta/is_vector_like.hpp

If that goofs your stuff I can put out a PR that adds those back as empty files

@bgoodri Do you see a possibility how we could make Rstan’s constraints part of some of Stan-math tests?

I am not saying we should do that, but at least consider.

The test is basically running R CMD check for 80 packages, but usually running it for rstanarm is representative of the other 79. My guess is that @stevebronder’s PR is OK as long as the Stan functions are still the same but I would have to check.

No function names are changed/added/deleted in that PR ftr

We should consider a rstanarm R cmd check in our pipeline…

We have had one (and for rstan) but it has failed thousands of times in a row due to lack of write permisions (to install packages) by the Jenkins user.

Could you coordinate with @seantalts and @serban-nicusor to get the Jenkins issues sorted?

If there aren’t automated upstream tests, we’ll keep breaking the upstream inadvertently.