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

Ints and reals not promoted properly in complex function signatures #998

Closed
1 of 3 tasks
WardBrian opened this issue Oct 12, 2021 · 6 comments · Fixed by #1004
Closed
1 of 3 tasks

Ints and reals not promoted properly in complex function signatures #998

WardBrian opened this issue Oct 12, 2021 · 6 comments · Fixed by #1004
Assignees
Labels
bug Something isn't working typechecker

Comments

@WardBrian
Copy link
Member

WardBrian commented Oct 12, 2021

Currently, this does not compile (throws a signature error):

generated quantities {
   complex x = add(1, 3i);
}

This creates a C++ error:

real x = norm(1.0);
  • SignatureMismatch does not recognize promotion from int/real to complex as valid (see Allow complex promotion in functions, not just operators #999 )
  • C++ does not promote double to std::complex<T> in function calls
  • Update tests/integration/good/code-gen/complex_scalar.stan to verify all complex functions, use of doubles and ints in function calls; remove explicit casts
@WardBrian WardBrian added bug Something isn't working typechecker labels Oct 12, 2021
@WardBrian
Copy link
Member Author

WardBrian commented Oct 12, 2021

It seems like the issue comes from this code in SignatureMismatch:

let rec check_same_type depth t1 t2 =
let wrap_func = Option.map ~f:(fun e -> TypeMismatch (t1, t2, Some e)) in
match (t1, t2) with
| t1, t2 when t1 = t2 -> None
| UnsizedType.(UReal, UInt) when depth < 1 -> None

Adding

  | UnsizedType.(UComplex, UReal) when depth < 1 -> None
  | UnsizedType.(UComplex, UInt) when depth < 1 -> None

resolves this issue.
@nhuurre - could you confirm this seems correct? I don't entirely understand this code

Should we be using UnsizedType.common_type or something from that module instead?

@WardBrian WardBrian changed the title Issue with signatures for add etc for complex numbers Ints and reals not promoted properly in function signatures Oct 13, 2021
@WardBrian
Copy link
Member Author

@SteveBronder can you help with the second bullet point here? We can use static_cast but will need to cast to the correct thing, either double or var, depending on context.

@nhuurre
Copy link
Collaborator

nhuurre commented Oct 13, 2021

The only difference between UnsizedType and SignatureMismatch versions of check_compatible_arguments_mod_conv should be that one returns a boolean (yes/no answer) and the other constructs a type mismatch info object (for printing a detailed error).
The depth parameter is not visible outside the module. It is only used to control the behavior when check_compatible_arguments recurses into function types.

It is indeed quite awkward to have two separate functions that do the same thing.

@WardBrian
Copy link
Member Author

The UnsizedType version seems to only be used to check operators like =-*/ etc. We could probably replace it with the SignatureMismatch version, though at the moment that creates a circular dependency. I'll open a separate issue about that, the rest of this seems more pressing currently

@nhuurre
Copy link
Collaborator

nhuurre commented Oct 13, 2021

Adding
| UnsizedType.(UComplex, UReal) when depth < 1 -> None
| UnsizedType.(UComplex, UInt) when depth < 1 -> None
resolves this issue.

Yes, that's correct as far as SignatureMismatch is concerned. Looks like C++ does not auto-convert these correctly, reminds me of #373.

@WardBrian
Copy link
Member Author

Yes, its a similar problem to what's preventing us from making arrays truly covariant - C++'s template resolution for function calls doesn't handle all these promotions the way we'd like.

@WardBrian WardBrian linked a pull request Oct 13, 2021 that will close this issue
2 tasks
@WardBrian WardBrian changed the title Ints and reals not promoted properly in function signatures Ints and reals not promoted properly in complex function signatures Oct 13, 2021
@WardBrian WardBrian linked a pull request Jan 6, 2022 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working typechecker
Projects
None yet
4 participants