

Marco,
I've been working through the static analyzer warnings about the Octave
code base and I ran in to a bit of trouble with eigs.
A sample warning is
__eigs__.cc (369) V550 An odd precise comparison: tmp.double_value() !=
0.0. It's probably better to use a comparison with defined precision:
fabs(A  B) > Epsilon.
for which the code is
tmp = map.getfield ("cholB");
if (tmp.is_defined ())
cholB = tmp.double_value () != 0.0;
Checking the documentation for eigs
'cholB'
Flag if 'chol (B)' is passed rather than B. The default is
false.
So, I thought the input was just a true/false and that I could change the
code to
cholB = tmp.bool_value ();
But now when I run the BIST tests I get
octave:1> test eigs
***** testif HAVE_ARPACK, HAVE_UMFPACK
A = toeplitz (sparse (1:10));
B = toeplitz (sparse ([1, 1], [1, 2], [2, 1], 1, 10));
R = chol (B);
opts.cholB = R;
[v, d] = eigs (A, R, 4, "lm", opts);
for i = 1:4
assert (A * v(:,i), d(i, i) * B * v(:,i), 1e12)
endfor
!!!!! test failed
octave_base_value::bool_value(): wrong type argument 'sparse matrix'
If I execute the commands individually I find that opts.cholB is not
true/false, but a sparse matrix.
Compressed Column Sparse (rows = 10, cols = 10, nnz = 19 [19%])
(1, 1) > 1.4142
(1, 2) > 0.70711
(2, 2) > 1.2247
(2, 3) > 0.81650
(3, 3) > 1.1547
(3, 4) > 0.86603
(4, 4) > 1.1180
(4, 5) > 0.89443
(5, 5) > 1.0954
(5, 6) > 0.91287
(6, 6) > 1.0801
(6, 7) > 0.92582
(7, 7) > 1.0690
(7, 8) > 0.93541
(8, 8) > 1.0607
(8, 9) > 0.94281
(9, 9) > 1.0541
(9, 10) > 0.94868
(10, 10) > 1.0488
Can you describe what is supposed to take place here and how to modify the
BIST tests? Is it as simple as "opts.cholB = true;"?
Rik

Administrator

On 1/7/19 4:48 PM, Rik wrote:
> Marco,
>
> I've been working through the static analyzer warnings about the Octave
> code base and I ran in to a bit of trouble with eigs.
>
> A sample warning is
>
> __eigs__.cc (369) V550 An odd precise comparison: tmp.double_value() !=
> 0.0. It's probably better to use a comparison with defined precision:
> fabs(A  B) > Epsilon.
>
> for which the code is
>
> tmp = map.getfield ("cholB");
> if (tmp.is_defined ())
> cholB = tmp.double_value () != 0.0;
>
> Checking the documentation for eigs
>
> 'cholB'
> Flag if 'chol (B)' is passed rather than B. The default is
> false.
>
> So, I thought the input was just a true/false and that I could change the
> code to
>
> cholB = tmp.bool_value ();
>
> But now when I run the BIST tests I get
>
> octave:1> test eigs
> ***** testif HAVE_ARPACK, HAVE_UMFPACK
> A = toeplitz (sparse (1:10));
> B = toeplitz (sparse ([1, 1], [1, 2], [2, 1], 1, 10));
> R = chol (B);
> opts.cholB = R;
> [v, d] = eigs (A, R, 4, "lm", opts);
> for i = 1:4
> assert (A * v(:,i), d(i, i) * B * v(:,i), 1e12)
> endfor
> !!!!! test failed
> octave_base_value::bool_value(): wrong type argument 'sparse matrix'
>
> If I execute the commands individually I find that opts.cholB is not
> true/false, but a sparse matrix.
It seems wrong to set opts.cholB to the sparse matrix itself here.
The double_value method for a sparse matrix just returns the first (0,0)
element of the matrix. That's pretty strange, so I'm wondering whether
this method should be deprecated and removed. There is no corresponding
bool_value method for sparse matrices, so you get the error.
But if the intent of this code is correct, the opts.cholB setting would
need to be
opts.cholB = R(0,0) != 0;
to avoid changing the meaning. And that seems odd. Is there something
special about chol(B) having a nonzero first element? If this really
was intended, it would be helpful to have a comment explaining why. And
to still use this code to make the intent explicit rather than passing R
and relying on the double_value method to extract the first element.
BTW, this warning from the static analyzer tool thing can generate a lot
of noise. There's nothing essentially wrong with testing a floating
point value is exactly zero (or not).
jwe


On 01/07/2019 02:35 PM, John W. Eaton wrote:
> On 1/7/19 4:48 PM, Rik wrote:
>> Marco,
>>
>> I've been working through the static analyzer warnings about the Octave
>> code base and I ran in to a bit of trouble with eigs.
>>
>> A sample warning is
>>
>> __eigs__.cc (369) V550 An odd precise comparison: tmp.double_value() !=
>> 0.0. It's probably better to use a comparison with defined precision:
>> fabs(A  B) > Epsilon.
>>
>> for which the code is
>>
>> tmp = map.getfield ("cholB");
>> if (tmp.is_defined ())
>> cholB = tmp.double_value () != 0.0;
>>
>> Checking the documentation for eigs
>>
>> 'cholB'
>> Flag if 'chol (B)' is passed rather than B. The default is
>> false.
>>
>> So, I thought the input was just a true/false and that I could change the
>> code to
>>
>> cholB = tmp.bool_value ();
>>
>> But now when I run the BIST tests I get
>>
>> octave:1> test eigs
>> ***** testif HAVE_ARPACK, HAVE_UMFPACK
>> A = toeplitz (sparse (1:10));
>> B = toeplitz (sparse ([1, 1], [1, 2], [2, 1], 1, 10));
>> R = chol (B);
>> opts.cholB = R;
>> [v, d] = eigs (A, R, 4, "lm", opts);
>> for i = 1:4
>> assert (A * v(:,i), d(i, i) * B * v(:,i), 1e12)
>> endfor
>> !!!!! test failed
>> octave_base_value::bool_value(): wrong type argument 'sparse matrix'
>>
>> If I execute the commands individually I find that opts.cholB is not
>> true/false, but a sparse matrix.
>
> It seems wrong to set opts.cholB to the sparse matrix itself here.
>
> The double_value method for a sparse matrix just returns the first (0,0)
> element of the matrix. That's pretty strange, so I'm wondering whether
> this method should be deprecated and removed. There is no corresponding
> bool_value method for sparse matrices, so you get the error.
>
> But if the intent of this code is correct, the opts.cholB setting would
> need to be
>
> opts.cholB = R(0,0) != 0;
>
> to avoid changing the meaning. And that seems odd. Is there something
> special about chol(B) having a nonzero first element? If this really
> was intended, it would be helpful to have a comment explaining why. And
> to still use this code to make the intent explicit rather than passing R
> and relying on the double_value method to extract the first element.
That's why I think this has a simple fix of replacing 'R' with "true"
because the other interpretation, that the first value of the matrix is
somehow exceedingly significant, doesn't make sense.
>
> BTW, this warning from the static analyzer tool thing can generate a lot
> of noise. There's nothing essentially wrong with testing a floating
> point value is exactly zero (or not).
I agree that this particular warning produces a lot of false positives to
work through. In this case it was useful for detecting something funny,
but most of the time it is noise.
Rik


On Mon, 7 Jan 2019 17:35:44 0500
"John W. Eaton" < [hidden email]> wrote:
> On 1/7/19 4:48 PM, Rik wrote:
> > Marco,
> >
> > I've been working through the static analyzer warnings about the Octave
> > code base and I ran in to a bit of trouble with eigs.
> >
> > A sample warning is
> >
> > __eigs__.cc (369) V550 An odd precise comparison: tmp.double_value() !=
> > 0.0. It's probably better to use a comparison with defined precision:
> > fabs(A  B) > Epsilon.
> >
> > for which the code is
> >
> > tmp = map.getfield ("cholB");
> > if (tmp.is_defined ())
> > cholB = tmp.double_value () != 0.0;
> >
> > Checking the documentation for eigs
> >
> > 'cholB'
> > Flag if 'chol (B)' is passed rather than B. The default is
> > false.
> >
> > So, I thought the input was just a true/false and that I could change the
> > code to
> >
> > cholB = tmp.bool_value ();
> >
> > But now when I run the BIST tests I get
> >
> > octave:1> test eigs
> > ***** testif HAVE_ARPACK, HAVE_UMFPACK
> > A = toeplitz (sparse (1:10));
> > B = toeplitz (sparse ([1, 1], [1, 2], [2, 1], 1, 10));
> > R = chol (B);
> > opts.cholB = R;
> > [v, d] = eigs (A, R, 4, "lm", opts);
> > for i = 1:4
> > assert (A * v(:,i), d(i, i) * B * v(:,i), 1e12)
> > endfor
> > !!!!! test failed
> > octave_base_value::bool_value(): wrong type argument 'sparse matrix'
> >
> > If I execute the commands individually I find that opts.cholB is not
> > true/false, but a sparse matrix.
>
> It seems wrong to set opts.cholB to the sparse matrix itself here.
Yes, it is wrong.
Marco

