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

remove var_alloc_stack_ and dependencies #565

bob-carpenter opened this Issue Jun 21, 2017 · 1 comment


None yet
4 participants

bob-carpenter commented Jun 21, 2017


Replace var_alloc_stack_ data structure and rewrite functions that depend on it

  • replace all the functions that depend on var_alloc_stack_ with explicit allocations wrapped in Map
    • rev/mat/fun/LDLT_alloc.hpp
    • rev/mat/fun/mdivide_left_ldlt.hpp
    • rev/mat/fun/mdivide_left_spd.hpp
    • rev/mat/fun/quad_form.hpp
    • rev/mat/fun/quad_form_sym.hpp
    • rev/mat/fun/trace_gen_quad_form.hpp
    • rev/mat/fun/trace_inv_quad_form_ldlt.hpp
    • rev/mat/fun/trace_quad_form.hpp
    • rev/mat/fun/log_determinant_ldlt.hpp
  • in rev/core/autodiffstackstorage.hppremove the member variable AutodiffStackStorage::var_alloc_stack_
  • remove file rev/core/chainable_alloc.hpp
  • add doc to the memory classes for reverse-mode autodiff
  • remove calls made redundant in rev/core/recover_memory.hpp and rev/core/recover_memory_nested.hpp
  • remove includes for removed removed files in
    • rev/core/grad.hpp
    • rev/core/set_zero_all_adjoints.hpp
    • rev/core/set_zero_all_adjoints_nested.hpp
    • rev/core.hpp


This will make the autodiff infrastructure simpler and a bit faster, because there'll only be two stacks to clean.

Current Version:


@bob-carpenter bob-carpenter added this to the 2.17 milestone Jun 21, 2017

@bob-carpenter bob-carpenter self-assigned this Jun 21, 2017

@bob-carpenter bob-carpenter changed the title from remove var_alloc_stack and associated pointers to remove var_alloc_stack_ and dependencies Jun 21, 2017

@seantalts seantalts modified the milestones: 2.17.0, 2.17.0++ Sep 6, 2017

@syclik syclik modified the milestones: 2.18.0, 2.18.0++ Feb 13, 2018

bbbales2 added a commit that referenced this issue Jul 5, 2018

Initial swing at removing the dependency on the C++ heap of a bunch o…
…f varis (so they don't need destructed) (Issue #565)

bbbales2 added a commit that referenced this issue Jul 5, 2018

bbbales2 added a commit that referenced this issue Jul 17, 2018

bbbales2 added a commit that referenced this issue Jul 18, 2018

Fixed a segfault in g++ and fixed some tests that used ldlts to not b…
…low up cause the ldlt was being called on a non-positive definite matrix (Issue #565)

This comment has been minimized.


bbbales2 commented Sep 11, 2018

Pull req. #927 (from branch feature/issue-565-vari-dealloc) removes the dependence of many of the varis that depended on chainable_alloc.

The issue with that pull request now is that in fixing these problems, it introduced some amount of complexity in the varis that changed that would not be necessary if they were re-implemented with adj_jac_apply.

The cleanup of LDLT_factor is worth keeping, and the cleanup and re-organization of tests is worthwhile as well.

For that pull request to be finished, what needs to happen is:

  1. LDLT_factor currently has two constructors. One that takes and Eigen matrix and immediately computes the decomposition, and one that takes no arguments, and an Eigen matrix is passed to it later to be decomposed. The second should be removed so that the LDLT object always exists in good state (or has thrown an exception). This removes the need for any checks that the LDLT object is usable or not.

  2. Remove stan/math/rev/core/build_double_array.hpp and stan/math/rev/core/build_vari_pointer_array_if_necessary.hpp from the pull request. This is the unnecessary complexity. Rewrite any of the functions that depend on them using adj_jac_apply (probably the simplest option) or other techniques. This may include functions in:

stan/math/rev/mat/fun/trace_inv_quad_form_ldlt.hpp and

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