-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Feature/issue 2778 add add diag #2779
Conversation
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01) |
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.
Thanks. The only thing this needs is a few more instantiations in the tests. The reason we require that is we want to make sure the math lib is doing the right thing in terms of being able to compile all possible signatures the language will throw at it. Parameters provide var
and data provide double
.
vector[N] vec; | ||
int scalar; | ||
} | ||
transformed data { |
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.
This file should have tests for all possible combinations of parameter and data for all possible signatures. That's a total of 3 signatures * 4 instantiations each (data/data, data/parameter, parameter/data, parameter/parameter).
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
Can someone remind which is better, greater or less than one. And also, what's the range of variation we expect. We don't want to induce 1% performance regressions on PRs, but how much of this is noise? |
Numbers greater than 1 indicate a speed up...but smaller than 3% changes are probably noise anyway. |
Is this ready? |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Add the add_diag function to the language.
Intended Effect
Less for loops.
How to Verify
make stan/src/test/test-models/good/function-signatures/math/matrix/add_diag
with command stan.
Side Effects
None.
Documentation
I'll open up an issue to add to docs when this is merged.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Copyright 2019 Andre Zapico
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: