EIGEN_NO_DEBUG rabbit hole (LDLT, factor_U, append_col/row, ...)

[Bob Carpenter]

At some point, our code diverged from being compatible with
Eigen debugging. The develop branch only passes tests
if -DEIGEN_NO_DEBUG is part of CFLAGS.

Can someone explain why this restriction got added to the code? I'd
like to make sure it's absolutely necessary. I don't think we have the
resources to maintain two versions of the code, but I hate to cut
off the option of running with range checks.

The specific problem I'm having is that it's very very useful for
debugging to run without disabling Eigen's debugging.



Daniel and I have started to fix these issues on:

bugfix/issue-904-eigen-assert-bugs

The LDLT test code indicated it wouldn't run otherwise.

append_row and append_col had broken tests with mismatches sizes
which must have only run by overwriting memory without segfaulting,
but there's no indication of why. I ifdefed out the test.

There are lots of these cases, but some of them may be dependent
on others.

I'd like to fix all of these and where it's not possible, indicate
why in the function and make sure the test code has an #ifdef so
I can run tests without disabling the debug flag.

- Bob

[Ben Goodrich]

On Tuesday, August 26, 2014 8:35:26 PM UTC-4, Bob Carpenter wrote:

Can someone explain why this restriction got added to the code?

We decided to do all the range-checking in Stan code, so the range-checking in Eigen was redundant. But that meant we can't turn off range-checking in the Stan code even when we want to.
 

Daniel and I have started to fix these issues on:

  bugfix/issue-904-eigen-assert-bugs

The LDLT test code indicated it wouldn't run otherwise.

I pushed a fix into that branch. The test made no sense.

Ben

[Bob Carpenter]

On Aug 26, 2014, at 10:04 PM, Ben Goodrich <goodri...@gmail.com> wrote:

> On Tuesday, August 26, 2014 8:35:26 PM UTC-4, Bob Carpenter wrote:

> Can someone explain why this restriction got added to the code?
>
> We decided to do all the range-checking in Stan code, so the range-checking in Eigen was redundant.
> But that meant we can't turn off range-checking in the Stan code even when we want to.

The question was why doesn't the code run without turning
off range checking? Where do the built-in range checks
get violated?

I just wanted to make sure whatever broke Eigen's range
checking was intended. It wasn't in the other functions I
looked at, just bugs that got through.

> The LDLT test code indicated it wouldn't run otherwise.
>
> I pushed a fix into that branch. The test made no sense.

Thanks.

- Bob

[Ben Goodrich]

On Tuesday, August 26, 2014 10:29:46 PM UTC-4, Bob Carpenter wrote:

> We decided to do all the range-checking in Stan code, so the range-checking in Eigen was redundant.
> But that meant we can't turn off range-checking in the Stan code even when we want to.

The question was why doesn't the code run without turning
off range checking?  Where do the built-in range checks
get violated?

The old version of the test passed by reference K - 1 CPCs when it was supposed to pass in (K * (K - 1)) / 2 CPCs, so the correctly-working factor_U() function wrote past the end of the vector.

We seem to have another failure in BFGS update. I'll look at it.

Ben

[Ben Goodrich]

On Wednesday, August 27, 2014 12:37:12 AM UTC-4, Ben Goodrich wrote:

We seem to have another failure in BFGS update. I'll look at it.

This seems to be a more serious problem. In

src/stan/optimization/bfgs_update.hpp

      inline void search_direction(VectorT &pk, const VectorT &gk) const {
        pk.noalias() = -(_Hk*gk);
      }

    private:
      HessianT _Hk;

the search_direction method is getting called when _Hk is uninitialized, meaning that it is a 0x0 matrix. So, with asserts disabled, it allows _Hk*gk to happen when gk is of length K > 0.

The real problem seems to be the default template thing:

 namespace stan {
   namespace optimization {
     template<typename Scalar = double,
-             int DimAtCompile = Eigen::Dynamic>
+             int DimAtCompile = -1>
     class BFGSUpdate_HInv {

since Eigen::Dynamic is an alias for zero. Changing it to -1 might break some stuff, but it should be safe.

We should unit-test with Eigen asserts on but compile users' models with them off.

Ben

[Marcus Brubaker]

I had always thought Eigen::Dynamic was equal to -1?

Anyway, I just stumbled upon that failing test the other day myself when working on cmake.  The real issue is that the test doesn't really make any sense, in practice search_direction is never called without at least one call to update.

Also, agreed, all testing should be done with all asserts/bound checks/etc turned on but models shouldn't be compiled that way by default.

Cheers,

Marcus

--
You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ben Goodrich]

On Wednesday, August 27, 2014 1:20:42 AM UTC-4, Marcus Brubaker wrote:

I had always thought Eigen::Dynamic was equal to -1?

You're right. It's defined in lib/eigen_3.2.0/Eigen/src/Core/util/Constants.h
 

Anyway, I just stumbled upon that failing test the other day myself when working on cmake.  The real issue is that the test doesn't really make any sense, in practice search_direction is never called without at least one call to update.

Yeah, I am just going to push a fix to the test that specifies the problem size instead of relying on the default.

Ben

[Daniel Lee]

On Wed, Aug 27, 2014 at 1:31 AM, Ben Goodrich <goodri...@gmail.com> wrote:

Anyway, I just stumbled upon that failing test the other day myself when working on cmake.  The real issue is that the test doesn't really make any sense, in practice search_direction is never called without at least one call to update.

Yeah, I am just going to push a fix to the test that specifies the problem size instead of relying on the default.

I already pushed a fix to that test last night -- I just called update() once before running search_direction(). I don't know if that's the same fix you were making, Ben, but feel free to change.

[Ben Goodrich]

I saw that when my initial push failed. I think it is still better to call the constructor with the template parameters specified, but it doesn't matter that much now. Anyway, the main thing left to do is to add -DEIGEN_NO_DEBUG to CFLAGS only when compiling users' models.

Ben
 

[Daniel Lee]

Ben, you're right: it's more of a software design issue. There is no explicit constructor on BFGSUpdate_HInv. It gets created in an inconsistent state.

If we remove -DEIGEN_NO_DEBUG from CFLAGS in Stan, I think we're fine. CmdStan, RStan, and PyStan have their own flags.

--

[Jiqiang Guo]

On Wed, Aug 27, 2014 at 10:35 AM, Daniel Lee <bea...@alum.mit.edu> wrote:

If we remove -DEIGEN_NO_DEBUG from CFLAGS in Stan, I think we're fine. CmdStan, RStan, and PyStan have their own flags.

not that related to the discussion.  but I think this flag has never been in rstan. 

[Bob Carpenter]

I agree that we should use the debug mode for tests then turn
it off at run time. The reason is that goal is to catch programming
mistakes, not run-time errors. If the latter happen, they're
also programming mistakes.

Yes, Eigen::Dynamic is -1, but if you call the default no-arg
constructor you get a 0 x 0 matrix. You can then assign different
dimensionality matrices to it.

- Bob