-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Improving external C++ integration #1278
Comments
@WardBrian and I chatted about this in person. My main comment was that I don't like something that changes language behavior being defined as an attribute. That's when Brian proposed the alternative,
which I can live with. |
This sounds great to me, simpler external c++ support would be fantastic! The only possible conflict I see is that we end up with two different approaches for external code (c++ vs stan):
and
Which could lead to some user confusion. I don't have any good suggestions/resolutions, but just flagging |
The user may want to import multiple functions from the same file so I'd suggest Is there a reason to include user code directly in the model namespace instead of letting the user choose their own namespace and then having So here's how it could look like: extern "usersource.hpp" namespace "whatever::name" {
real foo(real x, data array[] real z);
array[] int bar(array[] int y);
}
functions { }
model { } transpiles to #include <stan/model/model_header.hpp>
#include <usersource.hpp>
namespace model_namespace {
using whatever::name::foo;
using whatever::name::bar;
class model {
// ...
}
} It might be helpful to have a template of what Stan expects the C++ to look like. Stanc3 could have #include <stan/math.hpp>
namespace whatever {
namespace name {
template <typename T0__,
stan::require_all_t<stan::is_stan_scalar<T0__>>* = nullptr>
stan::promote_args_t<T0__>
foo(const T0__& x, const std::vector<double>& z, std::ostream* pstream__);
std::vector<int> bar(const std::vector<int>& y, std::ostream* pstream__);
}
} Finally, @andrjohns bring up a good point: importing functions from I think a reasonable option is an extra functions "filename.stanfunctions" {
foo, bar, baz
}
functions { }
model { } |
This is true, but IMO I prefer to have the file name attached to each one and then just de-duplicated by the compiler. No need for a new block, and it's always clear which file to look into if you want to find a certain function.
The only reason not to do this is I could not think of a non-clunky way to have the user communicate what namespace they used in their code. If we can agree on one then I think we should do the code generation as you suggest. The extra block does make this a bit easier, but I'm still not sold on adding a 8th block to the language for this (relatively obscure) feature
This would be easy to add, but I'd argue it wouldn't really be that helpful. The way we generate user defined functions at the moment is very specific to both the overload logic we employ and because there are no autodiff specific specializations for UDFs. The signatures such a flag would generate would be useful for things like the Eigen templates, but if you're trying to write a function and a specialization for its gradient (which I think is a major use case) the signature needs to look more like those in the math library itself, not what we currently generate. To both your final point and @andrjohns, I think a more general mechanism than |
Introduction
After #1277, the only required use for forward declarations of functions is for using external C++ code. This feature as currently implemented is sub-par for a number of reasons:
--allow-undefined
)We generate a C++ declaration for these functions, which means users must use our complicated templates and backward compatibility breaks any time we change our code generation.Proposed change:
A new syntax, which looks like
(note,
extern
is already a reserved word in Stan).This syntax solves each of the 3 problems above:
myfile.hpp
directly into the generated C++ (more likely we will just do a C++-level#include
), so that this code will live inside the namespace.Considerations
extern "foo.py" ...
style which does the same thing. Using the filename rather than something likeextern "C++"
is to allow for item 3 above.#include
s, we may want those outside the namespace. Something like this seems like it could be handled on the command line, rather than in the language.Previous discussions:
Soliciting opinions @bob-carpenter @mitzimorris @rok-cesnovar @nhuurre
The text was updated successfully, but these errors were encountered: