trying to review 346 (pointers, mixing namespaces in files, etc.)

[Bob Carpenter]

I was trying to code review Marcus's pull request:

https://github.com/stan-dev/stan/pull/346/files

but got very confused by all the pointers flying around
in the very first file I looked at:

https://github.com/mbrubake/stan/blob/3d3b350cc407ea4feb5855ba853744c35d7a65ae/src/stan/agrad/rev/matrix/ldlt.hpp

For example, in LDLT_factor, there's a new in the second
constructor but there's no destructor in which it's freed.
Is that because it's going on the auto-diff memory alloc?

In mdivide_left_ldlt_alloc, it looks like there's a different
approach to memory management with a

boost::shared_ptr< Eigen::LDLT< Eigen::Matrix<double,R1,C1> > > _ldltP;

Maybe somebody else could review this code?

I'd rather not see code from different name spaces in the
same file. This ldlt.hpp file defines entries for stan::math
and stan::agrad. The point was that there shouldn't be any
dependency from math to agrad, so we could compile math separately.
We can't really do that if we mix stan::agrad and stan::math
definitions in a single file.

Given that we have outstanding bugs that already seem to be
malloc related, I'd like to be very careful before introduing
more pointer manipulation that isn't fully encapsulated.

- Bob

[Marcus Brubaker]

On Wed, Nov 13, 2013 at 8:49 PM, Bob Carpenter <ca...@alias-i.com> wrote:

I was trying to code review Marcus's pull request:

  https://github.com/stan-dev/stan/pull/346/files

but got very confused by all the pointers flying around
in the very first file I looked at:

https://github.com/mbrubake/stan/blob/3d3b350cc407ea4feb5855ba853744c35d7a65ae/src/stan/agrad/rev/matrix/ldlt.hpp

For example, in LDLT_factor, there's a new in the second
constructor but there's no destructor in which it's freed.
Is that because it's going on the auto-diff memory alloc?

That's right, the object allocated there is derived from a chainable_alloc which is allocated on the autodiff stack.

 

In mdivide_left_ldlt_alloc, it looks like there's a different
approach to memory management with a

  boost::shared_ptr< Eigen::LDLT< Eigen::Matrix<double,R1,C1> > > _ldltP;

Maybe somebody else could review this code?

I'll take a look into why I did that and reply.  I don't remember anymore.

I'd rather not see code from different name spaces in the
same file.  This ldlt.hpp file defines entries for stan::math
and stan::agrad.  The point was that there shouldn't be any
dependency from math to agrad, so we could compile math separately.
We can't really do that if we mix stan::agrad and stan::math
definitions in a single file.

This is a somewhat necessary evil unless I'm missing some template magic.  Basically, we need to be able to write

LDLT_factor<T> ldltObj;

in a function where T is a template parameter of either double or var and get the right class.  If there is some way to do this with LDLT_factor<double> and LDLT_factor<var> defined in different namespaces I'd be happy to rewrite it.

 

Given that we have outstanding bugs that already seem to be
malloc related, I'd like to be very careful before introduing
more pointer manipulation that isn't fully encapsulated.

Fair enough.

Cheers,

Marcus

[Marcus Brubaker]

On Thu, Nov 14, 2013 at 10:58 AM, Marcus Brubaker <marcus.a...@gmail.com> wrote:

On Wed, Nov 13, 2013 at 8:49 PM, Bob Carpenter <ca...@alias-i.com> wrote:

I was trying to code review Marcus's pull request:

  https://github.com/stan-dev/stan/pull/346/files

but got very confused by all the pointers flying around
in the very first file I looked at:

https://github.com/mbrubake/stan/blob/3d3b350cc407ea4feb5855ba853744c35d7a65ae/src/stan/agrad/rev/matrix/ldlt.hpp

For example, in LDLT_factor, there's a new in the second
constructor but there's no destructor in which it's freed.
Is that because it's going on the auto-diff memory alloc?

That's right, the object allocated there is derived from a chainable_alloc which is allocated on the autodiff stack.

 

In mdivide_left_ldlt_alloc, it looks like there's a different
approach to memory management with a

  boost::shared_ptr< Eigen::LDLT< Eigen::Matrix<double,R1,C1> > > _ldltP;

Maybe somebody else could review this code?

I'll take a look into why I did that and reply.  I don't remember anymore.

This is for the mdivide_left_ldlt_dv_vari case, i.e., computing inv(A)*b when A is a double matrix with the LDLT_factor computed and b is a var matrix.  Otherwise a copy of the factorization would be necessary for every call to mdivide_left_ldlt.

Cheers,

Marcus