Align cmdstan and rstan auto-generated code?

Hi!

Given that we can now extend with C++ code our own models, I found it somewhat annoying that cmdstan and rstan follow different conventions for the automatically generated C++ file. Specifically what caused problems in my case was the different model namespace names. So for foo.stan we get

rstan: model_foo_namespace
cmdstan: foo_model_namespace

The easiest fix I found was to adjust cmdstan makefiles like, i.e. by changing in make/models the line

    `$(WINE) bin$(PATH_SEPARATOR)stanc$(EXE) $(STANCFLAGS) $< --o=$@`

to
$(WINE) bin$(PATH_SEPARATOR)stanc$(EXE) $(STANCFLAGS) --name=model_$(*F) $< --o=$@

Any objections against that?

Best,
Sebastian

HI Sebastian,

I would think it’s better to either:

  1. fix RStan so it matches the defaults provided the parser
    or
  2. change the defaults in the parser.

Although it all accomplishes the same thing, I think putting in an additional hack to the makefile isn’t the right way to fix it. There’s just a risk it drifts again.

I agree with @syclik.

We can modify the compiler (and generator) to accept a namespace as an argument rather than generating it from the model name.

Then I’m not sure what the requirements are for RStan vs. CmdStan vs. PyStan. I would think they should all use the same .hpp file for a model if we can find something that works.

I have no preference here and what I did was a quick fix from a user perspective. I haven’t found an option in RStan to set the namespace name, so cmdstan was the easiest way to fix it.

I suggest that we quickly resolve this in the next meeting.

Along with this we should then also try to align how Rstan and how cmdstan include C++ code. Currently that is handled differently in the two interfaces and it took me some quircks to be able to include C++ code into cmdstan and RStan which works for both interfaces in the same way.

Sebastian

OK, we can discuss at the next meeting. I’m not sure there’s any official way to include extensions to Stan via C++ other than somehow managing to include the new source file in the build process. I don’t see how we could unify RStan (goes through BH, uses R’s build tools) and CmdStan (uses makefiles).

Ok. I will raise it. Essentially the major difference is that rstan puts the include inside the model namespace whereas cmdstan puts it external to the model namespace at the very end of the C++ file.

I have for myself worked around the issue by instructing rstan to include the string

}
#include "custom.hpp"
namespace model_ns {

Doing so make my custom.hpp show up in the same context in rstan and in cmdstan compiled models (and I can use the same custom.hpp for both interfaces).

This works, but it feels like an ugly hack.

Sebastian

The CmdStan behavior is worse because it uses -include when the model is compiled in the makefile, so the file gets included at the top. For most users, including it inside the model namespace makes it easier to get right.

Yeah, for most users it is probably what they want to be included inside the model namespace. For my applications, I needed to deal with the stan::math namespace and hence needed my include to show up outside of the model namespace. In addition I wanted my (autogenerated) code to be compatible with rstan and cmdstan without any further modifications.

I am not saying what rstan is doing is not OK… not at all (I can live with my hack), we may want to find a way that is consistent across interfaces. Seems like it hasn’t been discussed yet.

Sebastian

Sebastian’s right — we need included code to go into the stan::math namespace if we want argument-dependent lookup for autodiff.

I think we should figure out going forward how to deal with the namespace for the generated code. It’d be easy to make configurable within stanc—it could take a vector<string> spec for namespaces and then just generate the right code. It was never thought through more generally from the very first CmdStan, which just generated a single executable. We’ve moved away from that to making the code generate an .hpp file, but I think we should clean this all up.