Unit test LDLT_factor better and fix derivative bug

[mbrubake]

Checklist

mdivide_left_ldlt

• math header file in src/stan/math/matrix/
• math test in src/test/math/matrix/
• agrad header file in src/stan/agrad/rev/matrix/
• agrad test in src/test/agrad/rev/matrix/
• agrad derivative tests
• agrad finite diff tests

mdivide_right_ldlt

• math header file in src/stan/math/matrix/
• math test in src/test/math/matrix/
• agrad header file in src/stan/agrad/rev/matrix/ (don't need header file -- the one in src/stan/math/matrix is templated properly)
• agrad test in src/test/agrad/rev/matrix/
• agrad derivative tests
• agrad finite diff tests

log_determinant_ldlt

• math header file in src/stan/math/matrix/
• math test in src/test/math/matrix/
• agrad header file in src/stan/agrad/rev/matrix/
• agrad test in src/test/agrad/rev/matrix/
• agrad derivative tests (one of two tests have gradient tests)
• agrad finite diff tests

trace_gen_inv_quad_form_ldlt

• math header file in src/stan/math/matrix/
• math test in src/test/math/matrix/
• agrad header file in src/stan/agrad/rev/matrix/
• agrad test in src/test/agrad/rev/matrix/
• agrad derivative tests
• agrad finite diff tests

trace_inv_quad_form_ldlt

• math header file in src/stan/math/matrix/
• math test in src/test/math/matrix/
• agrad header file in src/stan/agrad/rev/matrix/
• agrad test in src/test/agrad/rev/matrix/
• agrad derivative tests
• agrad finite diff tests

This pull request:

• Adds unit tests for LDLT_factor (resolving issue LDLT_factor needs unit tests #45). The _ldlt variants of mdivide, log_determinant, trace_inv_quad_form and trace_gen_inv_quad_form are all now directly tested.
• Fixes the gradient bug in trace_inv_quad_form_ldlt
• Reenables the use of trace_inv_quad_form_ldlt in multi_normal
• Uses trace_inv_quad_form_ldlt in multi_student_t_log
• Adds gradient testing on multi_student_t_log

[syclik]

Jenkins, retest this please.

[stan-buildbot]

Test PASSed.
Refer to this link for build results: http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20Github%20Pull%20Requests/274/

[syclik]

@mbrubake, here's the minimum that needs to get done before this can be merged. I'll try to help out, but there's still a lot of work to be done.

At a high level, each function should be in its own header file, each header file should only be defining things in one namespace (excluding anonymous namespace functions), each math function should have tests, each agrad function should have value tests, derivative tests (if you can calculate it analytically), and finite difference tests for the gradients.

I'm going to add two task lists. The first is a list of things I've noticed in the existing files. The second task-list is a grid that should help us keep track of what has been done and what needs to get done.

TODO

• split src/stan/math/matrix/ldlt.hpp into multiple header files
  • src/stan/math/matrix/mdivide_left_ldlt.hpp
  • src/stan/math/matrix/log_determinant_ldlt.hpp
  • src/stan/math/matrix/trace_gen_inv_quad_form_ldlt.hpp
  • src/stan/math/matrix/trace_inv_quad_form_ldlt.hpp
• create and implement src/stan/mat h/matrix/mdivide_right_ldlt.hpp
• split src/stan/agrad/rev/matrix/ldlt.hpp into multiple header files:
  • src/stan/agrad/rev/matrix/mdivide_left_ldlt.hpp
  • src/stan/agrad/rev/matrix/mdivide_right_ldlt.hpp
  • src/stan/agrad/rev/matrix/log_determinant_ldlt.hpp
  • src/stan/agrad/rev/matrix/trace_gen_inv_quad_form_ldlt.hpp
  • src/stan/agrad/rev/matrix/trace_inv_quad_form_ldlt.hpp
• need to remove functions defined in namespace stan::math in src/stan/agrad/rev/matrix/ldlt.hpp.

[syclik]

I'm moving the checklist to the first comment of the pull request.

[mbrubake]

Ok, I'll try to do some of the splitting of files.

Do we really need finite diff tests if we have analytic derivative tests? That seems redundant.

Regarding the namespace issue in src/stan/agrad/rev/matrix/ldlt.hpp, @bob-carpenter sent an email about that last week or sometime around then. This is a somewhat necessary evil unless I'm missing some template magic. Basically, in a function like multi_normal_log we need to be able to write

LDLT_factor<T> ldltObj;

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 but I don't think the type scoping rules work that way. BTW, this is basically the same pattern that is used to define numerical traits for Eigen in src/stan/agrad/rev/matrix/Eigen_NumTraits.hpp

[syclik]

On Mon, Nov 18, 2013 at 1:23 PM, Marcus Brubaker
notifications@github.comwrote:

Ok, I'll try to do some of the splitting of files.

Do we really need finite diff tests if we have analytic derivative tests?
That seems redundant.

Yes, it's redundant, but yes, I think we should include them. They end up
catching errors in calculating derivatives, which has happened before.

Regarding the namespace issue in src/stan/agrad/rev/matrix/ldlt.hpp,
@bob-carpenter https://github.com/bob-carpenter sent an email about
that last week or sometime around then. This is a somewhat necessary evil
unless I'm missing some template magic. Basically, in a function like
multi_normal_log we need to be able to write

LDLT_factor ldltObj;

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 and LDLT_factordefined in different namespaces I'd be happy to rewrite it but I don't
think the type scoping rules work that way.

Yes, there's a way to do this a little cleaner. What needs to happen is
that they get written into the different namespaces. In the agrad tests,
make sure either an include to the appropriate file with the LDLT_factor is
included or include stan/agrad/var.hpp BEFORE including
stan/prob/distributions/multivariate/continuous/multi_normal.hpp. That will
then bring the definition of the LDLT_factor in that matches more finely
than the existing one. I think it might need to be brought in before the
other definition, but I can't remember how well argument dependent lookup
works for templates.

I'm pretty sure we used that "trick" in operands and partials to make that
behave properly.

C++ is hairy.

BTW, this is basically the same pattern that is used to define numerical

traits for Eigen in src/stan/agrad/rev/matrix/Eigen_NumTraits.hpp

I'll create an issue to break apart
src/stan/agrad/rev/matrix/Eigen_NumTraits.hpp. That file shouldn't be
creating a class in the stan::agrad namespace. The gevv_vvv_vari class can
just be moved.

[syclik]

@mbrubake, I just created a pull request to your forked branch. I'll go through and break apart the stan/agrad/rev/matrix/ldlt.hpp header file next. The pull request currently passes test-headers and test-unit. Can you write unit tests for:

• src/test/math/matrix/LDLT_factor_test.cpp
• src/test/math/matrix/log_determinant_ldlt_test.cpp
• src/test/math/matrix/trace_gen_inv_quad_form_ldlt_test.cpp
• src/test/math/matrix/trace_inv_quad_form_ldlt_test.cpp

[bob-carpenter]

Supersedes #45, which I'm closing.

[syclik]

@mbrubake, I updated the pull request with the agrad version split out. The pull request currently passes test-headers and test-unit. Can you write unit tests for:

• src/test/agrad/rev/matrix/LDLT_alloc_test.cpp
• src/test/agrad/rev/matrix/LDLT_factor_test.cpp
• src/test/agrad/rev/matrix/mdivide_left_ldlt_test.cpp
  • needs finite difference tests
• src/test/agrad/rev/matrix/mdivide_right_ldlt_test.cpp
  • needs all tests
• src/test/agrad/rev/matrix/log_determinant_ldlt_test.cpp
  • needs finite difference tests
• src/test/agrad/rev/matrix/trace_gen_inv_quad_form_ldlt_test.cpp
  • needs finite difference tests
• src/test/agrad/rev/matrix/trace_inv_quad_form_ldlt_test.cpp
  • needs finite difference tests

[syclik]

@syclik, create a new pull request with a branch within stan-dev.

[stan-buildbot]

• code review
  
  Can one of the admins verify this patch?