-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Complex containers #1133
Complex containers #1133
Conversation
@SteveBronder the above commit adds the test we looked at on Friday, if you still have the chance to look at stan-dev/math#2671 |
@WardBrian just put up the PR! Are you able to make a local cmdstan and try out stan-dev/math#2680? |
@SteveBronder I ran this branch with the pr here: https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FStanc3/detail/PR-1133/7/pipeline/ and everything is green! I would still like to see stan-dev/math#2676 get addressed whenever @rok-cesnovar has the chance before this would get merged, just to automatically test all the declared complex signatures. |
Will look at this tmrw! |
@SteveBronder did you get a chance to look at this? |
Sorry its on my plate today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me! Though I think we need to do one thing in mem_pattern
to check that any matrix or vector that is assigned to a complex matrix or vector is demoted to AoS
. Besides that I'd like to try to cleanup the requires a bit on the functions so we don't have so many as I worry that could hurt compile time a good bit. But I can't do that then this PR looks good
test/integration/good/code-gen/complex_numbers/basic_ops_mix.stan
Outdated
Show resolved
Hide resolved
This all looks good! I think it just needs the thing in mem_pattern then it's good to merge |
I'm not sure I understand what needs to change in mem_pattern to get the necessary behavior |
I'll take a look today. It's just that on the first pass we want to make sure that if a matrix/vector is being assigned to a complex matrix/vector then it is tagged as not possible to be an parameters {
matrix[10, 10] A_p;
}
transformed parameters {
complex_matrix[10, 10] A_complex_tp = A_p;
} https://github.com/stan-dev/stanc3/blob/master/src/analysis_and_optimization/Mem_pattern.ml#L357 |
I'd appreciate you taking a look, that part of the code is tough for me |
See bb816a5 I think that's all it needs so that it knows that is an object that cannot be promoted. Do you still need to add any new error messages specifically for complex types? |
I believe all the error messages should be good to go |
Looks like it needs a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Thanks @SteveBronder - I'll hit merge on this as soon as 2.29.2 is out and good to go |
Well, also after @bob-carpenter and I update the relevant docs |
Closes #1116 . This branch has gotten to a really good place. I believe with the exception of stan-dev/math#2671 that the basic tests I've written here should compile and pass.
Thanks to previous PRs like #1102, #1115, #1106, most of these changes are pretty "boring", mostly just doing the obvious things based on the existing container types and the promotion semantics.
Things this PR adds:
complex_matrix
,complex_vector
,complex_row_vector
types. The behavior and code generation of these largely mirrors the existing Eigen types.matrix
, etc. tocomplex_matrix
as necessary (e.g. in assignment, in function calls)add
,subtract
,multiply
,minus
,elt_multiply
,elt_divide
) and basic container functions (e.grows
and the like)Things this PR does not add:
pow
, which might work already, and things like FFTs, which will need math support. I leave these all for later PRs.multiply: (complex_vector, row_vector)=> complex_matrix
. NB: this does currently work, but it does so by promoting therow_vector
tocomplex_row_vector
. We can later change this for speedups.Submission Checklist
Release notes
Added complex containers:
complex_matrix
,complex_vector
, andcomplex_row_vector
with basic arithmetic support. More special functions and specializations will follow. Note that real-valued containers will be promoted to their complex variants as neededCopyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)