-
-
Notifications
You must be signed in to change notification settings - Fork 198
[WIP] Use auto and cleanup templated function calls in scalar probability distributions #1347
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
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… as broadcast" This reverts commit 8edb67d.
…stable/2017-11-14)
Collaborator
Author
|
Also ignore the first two commits here. I can cherry pick the others onto a separate branch if having the revert is annoying |
Collaborator
Author
|
🙏 Blessed by the distribution tests 🙏 Once everything is passing I'm going to break these up into smaller PRs that also do some cheap math optimizations. They were a bit easier to see once some tech debt was paid |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a cleanup PR that
replaces
return_type_tin the return part of the scalar probability distributions withautoand adds inline where it previously was notChanges the type trait
T_partials_returnin the PR toT_partialsand uses another aliasT_returnfor the catching return statements. There were a few places we were doingreturn 0.0which was fine without auto since it would just cast to the return type. But with auto we need to return the same type fromreturnA lot of the functions here would be something like
return normal_lpdf<propto, T_y, T_loc, T_scale>(...)But we can just let the compiler deduce those types with
return normal_lpdf<propto>(...)Makes
seq_scalar_viewconstThere's a few places where we can use RVO and return the same object. There's a lot of places where we can't use RVO without paying the cost of calling
buildonoperands_and_partials. But the places we don't use it tend to just be failure paths and idt we care about the performance of those paths.There was some weirdness in the return types for some of the distributions. Previously returning 0.0 for a function with
return_type_t<>as the return type would do a construct of the return type out of the value we returned (for example if return type isvarwe would get backvar(0.0)) With auto we have to be consistent with the return of the function so this addsT_return(error_case_value)to the if statementsTests
Refactor so I wouldn't expect any new tests though can add some if requested
Side Effects
Hopefully none!
Checklist
Math issue Update internals to use more modern c++ #1308
Copyright holder: Steve Bronder
the basic tests are passing
./runTests.py test/unit)make test-headers)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested