Skip to content
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

Add optims to detect when SoA matrices can be used #955

Merged
merged 96 commits into from
Jan 12, 2022

Conversation

SteveBronder
Copy link
Contributor

Ignore for now, I'm debugging a few things and will ping @rybern and @rok-cesnovar when this is ready for review / will fill out the top level comment describing the scheme and changes here.

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Release notes

Allows log_prob_impl to use Struct of Arrays type for reverse mode autodiff

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@@ -22,6 +22,7 @@ Show help
--version Display stanc version number
--name Take a string to set the model name (default = "$model_filename_model")
--O Allow the compiler to apply all optimizations to the Stan code.
-fno-soa Turn off the Struct of Arrays memory optimization.
Copy link
Member

Choose a reason for hiding this comment

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

We never really had a good plan for these stanc3 flags, so we are a bit all over the place with them.

I am not sure I have a better idea, but the -fno-X seems like a C++ compiler flag or is this style also common in other areas?

@WardBrian already brought up the idea of enabling individual optimization flags. Do we maybe want to have the optimization levels but then a flag to disable individual optimizatons?

Copy link
Member

Choose a reason for hiding this comment

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

We can also merge this without discussing this, there is plenty of time before the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes so I think what we want to do is

  1. Merge update ad-level so it uses a circular flowgraph #1079 which has some of the code in the PR that fixes something in the monotone framework
  2. Merge [WIP] Optimization level interface #549 so we have an O1
  3. Merge this and turn it on at O1

Personally I'd prefer if we merged this immediately after the latest release. While I'm confident in it, I'd rather have a release cycle on dev just to make sure try to find any weird quirks

Copy link
Member

Choose a reason for hiding this comment

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

Cool with that yeah, although if its hidden behind O1, its less of a problem.

@rok-cesnovar
Copy link
Member

The tests are now failing with:

n file included from /mnt/nvme1n1/workspace/stanc3_PR-955/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/prim/prob/lkj_corr_cholesky_log.hpp:6:
/mnt/nvme1n1/workspace/stanc3_PR-955/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/prim/prob/lkj_corr_cholesky_lpdf.hpp:24:3: error: no matching function for call to 'check_lower_triangular'
  check_lower_triangular(function, "Random variable", L_ref);
  ^~~~~~~~~~~~~~~~~~~~~~
../../test/integration/good/function-signatures/distributions/multivariate/continuous/lkj_corr_cholesky_log.hpp:266:32: note: in instantiation of function template specialization 'stan::math::lkj_corr_cholesky_lpdf<false, stan::math::var_value<Eigen::Matrix<double, -1, -1, 0, -1, -1>, void>, double>' requested here
      transformed_param_real = lkj_corr_cholesky_lpdf<false>(p_matrix,
                               ^
../../test/integration/good/function-signatures/distributions/multivariate/continuous/lkj_corr_cholesky_log.hpp:568:14: note: in instantiation of function template specialization 'lkj_corr_cholesky_log_model_namespace::lkj_corr_cholesky_log_model::log_prob_impl<false, false, Eigen::Matrix<stan::math::var_value<double, void>, -1, 1, 0, -1, 1>, Eigen::Matrix<int, -1, 1, 0, -1, 1>, nullptr, nullptr>' requested here
      return log_prob_impl<propto__, jacobian__>(params_r, params_i, pstream);
             ^
/mnt/nvme1n1/workspace/stanc3_PR-955/performance-tests-cmdstan/cmdstan/stan/src/stan/model/model_base_crtp.hpp:95:50: note: in instantiation of function template specialization 'lkj_corr_cholesky_log_model_namespace::lkj_corr_cholesky_log_model::log_prob<false, false, stan::math::var_value<double, void> >' requested here
    return static_cast<const M*>(this)->template log_prob<false, false>(theta,
                                                 ^
../../test/integration/good/function-signatures/distributions/multivariate/continuous/lkj_corr_cholesky_log.hpp:68:3: note: in instantiation of member function 'stan::model::model_base_crtp<lkj_corr_cholesky_log_model_namespace::lkj_corr_cholesky_log_model>::log_prob' requested here
  ~lkj_corr_cholesky_log_model() { }
  ^
/mnt/nvme1n1/workspace/stanc3_PR-955/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/prim/err/check_lower_triangular.hpp:27:13: note: candidate template ignored: requirement 'is_eigen<var_value<Matrix<double, -1, -1, 0, -1, -1>, void> >::value' was not satisfied [with T_y = stan::math::var_value<Eigen::Matrix<double, -1, -1, 0, -1, -1>, void>]
inline void check_lower_triangular(const char* function, const char* name,
            ^

@SteveBronder
Copy link
Contributor Author

Oh whoops yes lkj is not supported by the new matrix type yet

@SteveBronder
Copy link
Contributor Author

@rybern alright I think this is good to go!

I currently have things set to have this turned on by default at O0. I think what I'd like to do is have this happen at O1 only and merge before the release, then after the release I'd actually like to make O1 the default option. I like this scheme because then we get all our optimizations and we also get a whole release cycle with them on in master so that if there are any bugs we have a whole release cycle to find them.

Does that work for you?

@WardBrian
Copy link
Member

I like the idea of this / #1029 to be on in O1, but make O1 the default after the next release. I think O0 staying as a 'do absolutely nothing' option is valuable

@rybern
Copy link
Collaborator

rybern commented Jan 4, 2022

That scheme sounds good to me @SteveBronder. +1 for @WardBrian's comment about keeping O0.

The last thing I want to double check on is https://github.com/stan-dev/stanc3/pull/955/files#r713252249, because that'd potentially give correctness issues

@SteveBronder
Copy link
Contributor Author

Alrighty @rybern I just added the above tests, if everything compiles correctly on Jenkins then I'm going to remove soa from O0 and then I think we are good to go!

Copy link
Collaborator

@rybern rybern left a comment

Choose a reason for hiding this comment

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

@SteveBronder SteveBronder merged commit c58fcc1 into stan-dev:master Jan 12, 2022
@SteveBronder
Copy link
Contributor Author

SteveBronder commented Jan 12, 2022

D-O-N-E!!!

@andrjohns
Copy link
Contributor

Thanks all for sticking through this! The varmat future is bright!

@wds15
Copy link

wds15 commented Jan 12, 2022

WHOW!!! Can I use it now? How?? I wanna test!!

Seriously... is there a simple way to make a test drive with my favourite model? Just do -O1 as argument to the parser and that's it?

I am really looking forward to this!

@SteveBronder
Copy link
Contributor Author

Just O1 should do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants