Skip to content
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

[BUG] --standalone-functions ignores undefined functions (external c++) #1297

Closed
andrjohns opened this issue Mar 22, 2023 · 4 comments · Fixed by #1298
Closed

[BUG] --standalone-functions ignores undefined functions (external c++) #1297

andrjohns opened this issue Mar 22, 2023 · 4 comments · Fixed by #1298

Comments

@andrjohns
Copy link
Contributor

The --standalone-functions flag doesn't produce the same function code and // [[stan::function]] decorator if an external function is declared with the --allow-undefined flag.

For example, the following model:

functions {
  real retreal(real x) {
    return x;
  }
}

Parses to:

...

// [[stan::function]]
auto retreal(const double& x, std::ostream* pstream__ = nullptr)  
{
 return model_model_namespace::retreal(x, pstream__);
}

Whereas:

functions {
  real retreal(real x);
}

Parses to:

// Code generated by stanc v2.31.0
#include <stan/model/model_header.hpp>
namespace model_external_model_namespace {

using stan::model::model_base_crtp;
using namespace stan::math;


stan::math::profile_map profiles__;
static constexpr std::array<const char*, 1> locations_array__ = 
{" (found before start of program)"};


template <typename T0__,
          stan::require_all_t<stan::is_stan_scalar<T0__>>* = nullptr>
  stan::promote_args_t<T0__>
  retreal(const T0__& x, std::ostream* pstream__) ; 


}
;
@andrjohns
Copy link
Contributor Author

I think that the only thing needed is the // [[stan::function]] decorator above the placeholder/generic definition, no other processing or definitions should be required

@WardBrian
Copy link
Member

I think that the only thing needed is the // [[stan::function]] decorator above the placeholder/generic definition, no other processing or definitions should be required

The current standalone-functions functionality always exposes functions, not templates, and always puts them outside the model namespace, so adding a comment there would make this very different behavior.

The easiest "fix" for this is just making this match statement always equivalent to the current "Some" case:

match fdbody with
| None -> []
| Some _ ->

That will introduce a new issue where

functions {
  real retreal(real x);
  real retreal(real x) {
    return x;
  }
}

would now output duplicate definitions, but that should be fixable. I think this oversight is simply because we don't want to generate two signatures if it was a "normal" (i.e. not for external C++) forward decl, and right now it is annoying to tell these two cases apart (one motivation for ideas in #1278) .

@andrjohns
Copy link
Contributor Author

The current standalone-functions functionality always exposes functions, not templates, and always puts them outside the model namespace, so adding a comment there would make this very different behavior.

Ah gotcha. I've got no strong preference on a particular approach. It's for making external c++ compatible with expose_functions() in cmdstanr, so I'm happy with whatever works!

@WardBrian
Copy link
Member

I think I have a reasonable fix for this which also fixes another known issue with external C++ (we don’t generate the necessary functors to call them in Higher order functions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants