Skip to content

Conversation

@SteveBronder
Copy link
Collaborator

@SteveBronder SteveBronder commented Sep 12, 2019

Summary

This is a cleanup PR that

  1. replaces return_type_t in the return part of the scalar probability distributions with auto and adds inline where it previously was not

  2. Changes the type trait T_partials_return in the PR to T_partials and uses another alias T_return for the catching return statements. There were a few places we were doing return 0.0 which was fine without auto since it would just cast to the return type. But with auto we need to return the same type from return

  3. A 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>(...)
  1. Makes seq_scalar_view const

  2. There'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 build on operands_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.

  3. There was some weirdness in the return types for some of the distributions. Previously returning 0.0 for a function withreturn_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 is var we would get back var(0.0)) With auto we have to be consistent with the return of the function so this adds T_return(error_case_value) to the if statements

Tests

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

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@SteveBronder
Copy link
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

@SteveBronder SteveBronder changed the title Use auto and cleanup templated function calls in scalar probability distributions [WIP] Use auto and cleanup templated function calls in scalar probability distributions Sep 12, 2019
SteveBronder and others added 20 commits September 12, 2019 21:22
…/google/stable/2017-11-14)"

This reverts commit 95b5e5a.
This reverts commit cf842ea, reversing
changes made to a5376d2.
This reverts commit a5376d2.
@SteveBronder
Copy link
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

@serban-nicusor-toptal serban-nicusor-toptal added this to the 3.0.0 milestone Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants