stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.59k stars 369 forks source link

Order of checks in matrix functions #1415

Closed rtrangucci closed 9 years ago

rtrangucci commented 9 years ago

Checks in matrix functions not ordered correctly; e.g.

mdivide_left_spd.hpp:

stan::math::check_symmetric("mdivide_left_spd", "A", A);
stan::math::check_pos_definite("mdivide_left_spd", "A", A);
stan::math::check_square("mdivide_left_spd", "A", A);
stan::math::check_multiplicable("mdivide_left_spd", 
                                                   "A", A,
                                                   "b", b);

@syclik pointed out we should be checking whether the two matrices are multiplicable before we run the check_pos_definite test. Check_pos_definite also already checks symmetry internally, so no need to call it twice.

bob-carpenter commented 9 years ago

The positive definiteness check should also check that the matrix is square.

On Mar 31, 2015, at 10:33 PM, Rob Trangucci notifications@github.com wrote:

Checks in matrix functions not ordered correctly; e.g.

mdivide_left_spd.hpp:

stan::math::check_symmetric("mdivide_left_spd", "A", A); stan::math::check_pos_definite("mdivide_left_spd", "A", A); stan::math::check_square("mdivide_left_spd", "A", A); stan::math::check_multiplicable("mdivide_left_spd", "A", A, "b", b);

@syclik pointed out we should be checking whether the two matrices are multiplicable before we run the check_pos_definite test. Check_pos_definite also already checks symmetry internally, so no need to call it twice.

— Reply to this email directly or view it on GitHub.

rtrangucci commented 9 years ago

@bob-carpenter I should have added that check_pos_definite calls check_symmetric which subsequently calls check_square, so there only need to be two checks in mdivide_left_spd(A,b):

check_multiplicable(A,b)
check_pos_defininte(A)
syclik commented 9 years ago

This issue was moved to stan-dev/math#88