-
Notifications
You must be signed in to change notification settings - Fork 32
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
Optimise convolutions #745
Conversation
4eb2edd
to
bca39db
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Slightly surprisingly this doesn't seem to make a great deal of difference. Regardless I think it should be more stable and is definitely easier to test. |
Thanks for this, Sam. I'll review it today. |
How did you get on with this? |
Sorry, I missed this notification. I'll finish reviewing all your PRs by close of Thursday. I'm prioritising the GP vectorisations PR due to the speed gains. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 781a930 is merged into main:
|
Co-authored-by: James Azam <james.azam@lshtm.ac.uk>
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 all looks good to me. The code is more readable now.
Nice and I agree. Thanks for the review. This is just waiting on our decisions in the conversations. My vote is leave as is for this PR and if we feel strongly move to a new issue to refactor? It could be a good issue for someone new to stan to tackle if we decide to go that way |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 500924f is merged into main:
|
I don't feel strongly about it. |
Description
This PR is unlinked to an issue but applies some optimises to speed up convolutions (a key pain point of the stan code base). It also replaces the custom
reverse_mf
function with the newreverse
built in.Initial submission checklist
devtools::test()
anddevtools::check()
).devtools::document()
).lintr::lint_package()
).After the initial Pull Request