Error checking functions; logic in check_pos_definite(); origin of the domain_error() function signature

[Daniel Lee]

There were a bunch of questions on a pull request that I thought should be pulled out and not lost.

From Bob:

Why do these things all return bool values that are never used? None of them can return false. Should I start another issue for that or am I missing something?

Bob's referring to the check_* functions in the math library. The bulk of them are in stan/math/prim/scal/err/.

We should really change them to all be void functions. At some point, they used to return bools and we would use to the result to then throw from the calling function. At some point, I walked through all the functions and made them consistent. They all throw if the checks fail now. At the time I made the change, I didn't get around to fixing all the uses of the function, so I think we still have some of them in the conditions of if statements.

We can start a new issue. I have one open (math#50) for creating is_* functions that we can expose to the language.

I'm reluctant to change the error message given I don't know what other functions call this function. Isn't the second test here both expensive (probably not compared to Cholesky factoring, but still) and redundant? 

 

if (cholesky.info() != Eigen::Success || !(cholesky.matrixLLT().diagonal().array() > 0.0).all()) 

 

Wikipedia says you can only Cholesky factor a positive-definite matrix. 

 

And the first test uses "cholesky.isPositive()" and doesn't have the diagonal().array() > 0 business. Instead, it has: 

 

  || (cholesky.vectorD().array() <= 0.0).any()) 

 

which may not work if any of the results are NaN, because they'll return false. It then goes on to do check_not_nan() on the whole function, but the form used with the diagonal().array() will already check that, because any NaN will automatically return false, so the test will fail. 

 

If the isPositive() check is different than the diagonal().array() > 0 check, can we return a different and more relevant error message?

These are good questions. I'm not sure. Hopefully someone has answers. I think the logic in the matrix check functions could be revisited.

Also not sure how that passed lint because it had multiple lines longer than 80 chars.

No idea. I think cpplint skips comments. Not quite sure.

 

I'm also confused about the intended usage pattern of domain_error(); in the check_pos_definite file I found:

domain_error(function, "LDLT decomposition of", " failed", name); domain_error(function, name, "is not positive definite.", "");

and added

domain_error(function, "Matrix", " is not positive definite", name);

Also, I don't like having to pad one string and not the other---just seems like it's ripe for errors.

Yes. It's ripe for error. I didn't know what I was doing when I first consolidated all the functions. We definitely don't need to pad -- I think I did it that way cause of templated types. These had originally been the same interface as Boost's domain_error function. I think we can safely depart from that now.

Also, do I need to keep putting "fixes #xyz" in commits or does it just need to go in the pull request or what?

No need to put it in commits. As I merge, I add that to the merge commit and it automatically closes the issue. The thing that makes it easy is if the issue number is in the pull request. Either in the branch or somewhere that's easy to find.

Daniel

[Ben Goodrich]

On Friday, September 2, 2016 at 4:45:22 PM UTC-4, Daniel Lee wrote:

I'm reluctant to change the error message given I don't know what other functions call this function. Isn't the second test here both expensive (probably not compared to Cholesky factoring, but still) and redundant? 

 

if (cholesky.info() != Eigen::Success || !(cholesky.matrixLLT().diagonal().array() > 0.0).all()) 

 

Wikipedia says you can only Cholesky factor a positive-definite matrix. 

 

And the first test uses "cholesky.isPositive()" and doesn't have the diagonal().array() > 0 business. Instead, it has: 

 

  || (cholesky.vectorD().array() <= 0.0).any()) 

 

which may not work if any of the results are NaN, because they'll return false. It then goes on to do check_not_nan() on the whole function, but the form used with the diagonal().array() will already check that, because any NaN will automatically return false, so the test will fail. 

 

If the isPositive() check is different than the diagonal().array() > 0 check, can we return a different and more relevant error message?

These are good questions. I'm not sure. Hopefully someone has answers. I think the logic in the matrix check functions could be revisited.

The isPositive() method is defined for the LDLT class (not the LLT) class. Eigen permits an LDLT decomposition of positive semi-definite matrices or negative semi-definite matrices but not indefinite matrices. So, checking whether isPositive() is true is pretty useless if you are also going to check the diagonal of the D matrix, but if isPositive() is false (meaning the matrix is negative definite), then you just extract a bool instead of evaluating the signs of all the diagonal elements of D. Eigen doesn't worry much about NaNs, so it is possible that .info() yields success and isPositive() is true while some of the diagonal elements of D are NaN. The subsequent check_non_nan() is redundant.

Ben