Skip to content

Conversation

andrjohns
Copy link
Collaborator

Summary

This PR adds a framework for vectorising ternary functions, analogous to that of apply_scalar_binary. It is compatible with all scalar/container combinations, as well as arbitrarily nested containers.

The inc_beta function has been vectorised as an example function

Tests

Testing frameworks for both prim and mix have been added to ensure that both values and gradients are correct when using the vectorisation framework

Side Effects

N/A

Release notes

Added framework for vectorising ternary functions

Checklist

  • Math issue Vectorise ternary functions #2638

  • Copyright holder: Andrew Johnson

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • 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

@andrjohns
Copy link
Collaborator Author

@t4c1 There's an odd failure of the expression tests here:

[ RUN      ] ExpressionTestPrim.poisson_lcdf585
./test/expressions/expression_test_helpers.hpp:101: Failure
Expected equality of these values:
  a
    Which is: -726.65
  b
    Which is: -726.65

Any idea what this could be about?

@andrjohns
Copy link
Collaborator Author

Ah sorry for the ping, figured it out

@bob-carpenter
Copy link
Member

figured it out

Now I'm curious. I'm guessing the test uses more precision than the print.

If that's the case, maybe we could change the way it prints by default so this isn't so confusing.

@andrjohns
Copy link
Collaborator Author

Now I'm curious. I'm guessing the test uses more precision than the print.

Yep, there was a numerical difference because one of the captured scalars was falling out scope, but this wasn't shown by the print

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.56 3.56 1.0 0.15% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.98 -1.91% slower
eight_schools/eight_schools.stan 0.09 0.08 1.02 2.01% faster
gp_regr/gp_regr.stan 0.14 0.14 0.99 -0.89% slower
irt_2pl/irt_2pl.stan 5.77 5.69 1.01 1.39% faster
performance.compilation 92.92 91.25 1.02 1.8% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.03 8.11 0.99 -0.98% slower
pkpd/one_comp_mm_elim_abs.stan 31.75 32.03 0.99 -0.89% slower
sir/sir.stan 118.17 121.3 0.97 -2.65% slower
gp_regr/gen_gp_data.stan 0.03 0.03 1.0 -0.1% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.02 2.99 1.01 0.7% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.43 0.41 1.04 4.25% faster
arK/arK.stan 2.06 2.06 1.0 -0.24% slower
arma/arma.stan 0.23 0.23 1.0 0.49% faster
garch/garch.stan 0.58 0.57 1.0 0.48% faster
Mean result: 1.00268204141

Jenkins Console Log
Blue Ocean
Commit hash: e9fa1d7


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@andrjohns
Copy link
Collaborator Author

Closing for now to clear PR backlog, will re-open once ready for review

@andrjohns andrjohns closed this May 1, 2022
@andrjohns andrjohns reopened this Jun 22, 2022
@andrjohns
Copy link
Collaborator Author

@serban-nicusor-toptal There's an odd Git failure on the Jenkins run, any chance you've seen something like this before>

spinkney
spinkney previously approved these changes Nov 2, 2022
Copy link
Collaborator

@spinkney spinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great! Nice to see the expression updated with make_holder.

One question I have just to know.

  • How come we don't need a template for the binary in eigen_plugins.h?

Copy link
Collaborator

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple Qs but overall good!

@andrjohns
Copy link
Collaborator Author

@SteveBronder this is ready for another look. I ended up adding make_holder to all of the apply_scalar_binary calls, since I was still having issues with scalars falling out of scope

@andrjohns
Copy link
Collaborator Author

@SteveBronder sorry to double-ping, but I'm hoping to introduce an arbitrary vectorisation framework once this is in. Any chance you'd have time for another look?

@syclik syclik dismissed SteveBronder’s stale review May 23, 2023 13:28

Comments addressed. Will rereview.

Copy link
Collaborator

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small changes and Qs but overall looks good!

syclik
syclik previously requested changes May 23, 2023
Copy link
Member

@syclik syclik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking for some clarification.

Copy link
Collaborator

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@andrjohns andrjohns dismissed syclik’s stale review May 25, 2023 19:20

All addressed

@andrjohns andrjohns merged commit c71dd3e into stan-dev:develop May 25, 2023
@syclik
Copy link
Member

syclik commented May 25, 2023

@andrjohns, thanks! I'm glad we could pull out the additional Eigen plug-in! That simplifies the logic and the burden on developers is significantly lower. I think it'll be worth it in the long run.

There's some additional tech debt added from this PR. It was merged with British spelling of "specialization." I'll put up a PR to address it.

(Yes, choice of American vs British English as the default is arbitrary and wasn't really up for debate, but at this point it's better to be consistent rather than mixed.)

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.

7 participants