qr_R induces a segmentation fault

[betanalpha]

Description:

Running qr_R on the included example induces a segmentation fault in CmdStan.

Reproducible Steps:

fail_example.zip

> ./fail_example sample data file=fail_example.data.R

Current Output:

Segmentation fault: 11

Expected Output:

Should run without seg faulting.

Current Version:

develop

[bgoodri]

I have found the bug. The question becomes do we want qr_Q and qr_R to return the "thin" QR factors or do we want them to return the "fat" QR factors with the segfault averted. Doing the "thin" one is kind of what I had in mind (hence the segfault) but would be a backward incompatible change to the current (broken) behavior.

[betanalpha]

I would support either — or even both. Either way the doc
just needs to make it clear which is being applied and the
dimensions of the returned objects (which is missing from
the current doc).

On Apr 22, 2016, at 7:35 PM, bgoodri notifications@github.com wrote:

I have found the bug. The question becomes do we want qr_Q and qr_R to return the "thin" QR factors or do we want them to return the "fat" QR factors with the segfault averted. Doing the "thin" one is kind of what I had in mind (hence the segfault) but would be a backward incompatible change to the current (broken) behavior.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

[bgoodri]

In the interest of getting this into 2.10, I think we should leave it as fat until 3.0. If someone is only using qr_Q currently, then it is fat but not broken.

[bob-carpenter]

It's OK to deprecate or even remove broken functions, but
whatever we do, they can't segfault. That brings down the
R and Python process with it!

When you say the current behavior is broken, you just mean
for some inputs, or is it totally useless? If the former,
can we detect the cases that would segfault before they do
and throw an exception instead? If the latter, we should just
eliminate the function.

• Bob

On Apr 22, 2016, at 2:35 PM, bgoodri notifications@github.com wrote:

I have found the bug. The question becomes do we want qr_Q and qr_R to return the "thin" QR factors or do we want them to return the "fat" QR factors with the segfault averted. Doing the "thin" one is kind of what I had in mind (hence the segfault) but would be a backward incompatible change to the current (broken) behavior.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[bgoodri]

The segfault is readily fixable; it is just a question of whether to do so by breaking code that was using it without segfaulting. But we can wait for 3.0 to do that. Someone just needs to merge #289

[bob-carpenter]

I

On Apr 23, 2016, at 6:04 PM, bgoodri notifications@github.com wrote:

The segfault is readily fixable; it is just a question of whether to do so
by breaking code that was using it without segfaulting.

I wouldn't say that's a "fix", so much as a "change". But see below.

But we can wait for
3.0 to do that. Someone just needs to merge

#289

I'm very confused. This pull request says it

"Fix segfault when qr_R is called on a non-square matrix"

It doesn't mention breaking anything as a side effect.

So I don't see what's left to do for 3.0.

• Bob

[bgoodri]

On Sun, Apr 24, 2016 at 2:32 PM, Bob Carpenter notifications@github.com
wrote:

It doesn't mention breaking anything as a side effect.

So I don't see what's left to do for 3.0.

The PR doesn't break anything. For 3.0 we might consider a
backwards-incompatible change of returning the factors of the "thin" QR
decomposition of A where Q has the same dimensions as A and R is square. In
the current "fat" QR decomposition of A, Q is square and R has the same
dimensions as A.