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

Add stub functions for certain std:: functions #2611

Closed
1 of 5 tasks
WardBrian opened this issue Oct 25, 2021 · 10 comments · Fixed by #2612
Closed
1 of 5 tasks

Add stub functions for certain std:: functions #2611

WardBrian opened this issue Oct 25, 2021 · 10 comments · Fixed by #2612
Assignees

Comments

@WardBrian
Copy link
Member

WardBrian commented Oct 25, 2021

This is related to stan-dev/stanc3#997 / stan-dev/stanc3#1011

There are several functions which we don't define in stan::math but rather use std:: (or don't qualify them at all) for when the argument is only reals or ints. As far as I can tell, these are:

  • pow
  • fmod
  • atan2
  • min, max (the versions that accept 2 scalars, not the ones for arrays etc)
  • ceil

It would be great to redirect these within the library, so we can codegen all functions as fully qualified, e.g. stan::math::pow.

We may also need more definitions for norm and pow of complex numbers, see this log for what currently fails

@rok-cesnovar
Copy link
Member

Are you sure about ceil()?

Running this compiles fine for me:

#include <stan/math.hpp>
#include <iostream>

int main() {

   double a = 2.15;
   int b = 2;
   int a_ceil = stan::math::ceil(a);
   int b_ceil = stan::math::ceil(a);

   std::cout << a_ceil << ", " << b_ceil << std::endl;
}

So I guess the question is how do I test this? The stanc3 PR branch and try to compile the model I guess? For stuff like atan2 its clear there is no implementation.

@WardBrian
Copy link
Member Author

ceil, min, and max are all current special cased in the code gen. It's possible this special-casing isn't required, but it looks like this:

  | "ceil" ->
      let std_prefix_data_scalar f = function
        | [ Expr.({ Fixed.meta=
                      Typed.Meta.({adlevel= DataOnly; type_= UInt | UReal; _}); _
                  }) ] ->
            "std::" ^ f
        | _ -> "stan::math::" ^ f
      in
      Some
        (fun ppf es ->
          let f = std_prefix_data_scalar f es in
          pp_call ppf (f, pp_expr, es) )

@WardBrian
Copy link
Member Author

What I'll do is remove this special casing in the stanc3 branch and then we can test against that

@rok-cesnovar
Copy link
Member

Oh, thats probably there from before the various Math refactors, one of which also added support for that in ceil().

I can confirm easily for the rest with a simple program, ceil() seems to work fine.

@WardBrian
Copy link
Member Author

Removing the std:: special cases has lead to:

'../test/integration/good/int_overloads.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/min-max-types.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/optimization/rosenbrock.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/function-signatures/math/functions/pow.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/function-signatures/math/functions/min.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/function-signatures/math/functions/fmod.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/function-signatures/math/functions/atan2.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/function-signatures/math/functions/max.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/code-gen/complex_scalar.stan' had fails '[]' and errors '['Did not compile!']'
'../test/integration/good/code-gen/shadowing.stan' had fails '[]' and errors '['Did not compile!']'

so it seems like min and max do need this treatment, but ceil was indeed already done

@bob-carpenter
Copy link
Member

If we only have stan::math::foo(const var& x) defined, then a double will get promoted when called. We need to make sure the double-based implementations get called because promotion in this context is super inefficient.

@rok-cesnovar
Copy link
Member

Yeah, we do need to check that.

Given that we already have all the infrastructure to generate tests on-the-fly for expression testing, I think its worth it to actually make a test with all the scalar signatures and then after calling all the functions check that no var was created.

@bob-carpenter
Copy link
Member

@rok-cesnovar --- that sounds perfect. @syclik and I introduced tests like this ages ago and I don't even know if they're sitting around, but the memory manager lets you inspect the allocations if you need to write from scratch.

@WardBrian
Copy link
Member Author

@rok-cesnovar have you had a chance to look at this yet?

@rok-cesnovar
Copy link
Member

Yes, I am in the process of making a script to generate the tests mentioned above. I can fix the functions first if this is blocking you though. That can be done quick.

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.

3 participants