bugGNU Octave - Bugs: bug #43098, "if (array)" should...

 
 

bug #43098: "if (array)" should raise a warning, and give the same result for dense and sparse

Submitted by:  Colin Macdonald <cbm>
Submitted on:  Thu 28 Aug 2014 09:32:19 AM UTC  
 
Category:  Libraries Severity:  4 - Important
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  Lachlan Andrew <lachlan>
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 29 Jun 2016 09:57:22 PM UTC, comment #16: 

Thanks for the feedback and for applying the patch, Rik.

Yes, the username is set.  I was recently just posting the files from .hg/patch instead of "export -o", without realising that this meant the username was missing.  I'll regenerate the offending patches soon.

Lachlan Andrew <lachlan>In charge of this item.
Wed 29 Jun 2016 05:25:18 PM UTC, comment #15: 

@Lachlan: Okay, I applied your cset here (http://hg.savannah.gnu.org/hgweb/octave/rev/efce657ceb86).

I made a few changes, see the attached if_array.diff file.  It was small stuff, in one case the C++ function had a return prototype of bool but you were returning 0.  It seemed clearer to use false in this case.

Also, do you have the username variable set in your .hgrc?  The last two HG csets from you have not had the committer information, and I certainly want to give credit where it is due.

(file #37625)

Rik <rik5>
Project Administrator
Sat 25 Jun 2016 02:18:17 PM UTC, comment #14: 

Thanks for the feedback, Colin and Rik.  Each iteration I'm learning more about Octave :)

The new patch omits the brackets and spaces.  Calling X||0 does execute the same code (is_true()) as if(...).  The patch is to warn of the use of matrices as boolean values in all contexts: while, until, if, &&, || so I see each as being as explicit as another.  However, I suppose the bug report was about "if" only.

It now uses octave::math::isnan -- I didn't know about it before.

The sparse<bool> is now a specialization.

Another change that wasn't requested is in "is_equal" in ov.cc.  That is used in things like switch statements.  To avoid the warning, I now explicitly take "all()" before "is_true()", but have to reshape it so that all() captures all dimensions.  The alternative would be to have an argument to  is_true()  that requests that the warning be suppressed.  I'm not sure which would be more efficient.

(file #37574)

Lachlan Andrew <lachlan>In charge of this item.
Fri 24 Jun 2016 07:26:24 PM UTC, comment #13: 

Poor Lachlan, endless code review.  But, the code is definitely getting cleaner and tighter.  I had a question about testing for NaN values.

+bool
+Sparse<T>::SparseRep::any_element_is_nan (void) const
+{
+  octave_idx_type nz = nnz ();
+
+  for (octave_idx_type i = 0; i < nz; i++)
+    if (d[i] != d[i])
+      return true;
+
+  return false;
+}

You're relying on an IEEE floating point trick that NaN != NaN.  This works, but it is a little unclear.  Would it not be better to use octave::math::isnan() as is done elsewhere in the codebase?

Second, does it make sense to add a specialization for SparseBool matrices for this any_element_is_nan function?  For SparseBool, the element type T is bool which can never be NaN and therefore the function could return false immediately.

Rik <rik5>
Project Administrator
Fri 24 Jun 2016 06:22:06 PM UTC, comment #12: 

>  I still think it is useful to have a "once only" mode for warnings.


I personally also like this idea.  But I also think people prefer patches that do one thing right.  You can always prepare a separate patch for you "once only" idea.

These tests look reasonable.

  • I don't think "%! error (meh)" usually has those brackets.
  • There are some extraneous spaces in "[ 2i 4i ]".
  • I don't really know the internals: I guess you're saying that "array || 0" tests the same functionality as "if (array)"...  I would've explicit had:

    %!warning <boolean value implies all>
    %! if ([1 1 0])
    %!   assert (false)
    %! end

(but maybe its not a big deal).

Colin Macdonald <cbm>
Fri 24 Jun 2016 10:34:16 AM UTC, comment #11: 

I couldn't reproduce the failure with the natural syntax of the BIST, so the patch now uses the natural syntax.

The previous attempt to make the range case match  all(r!=0)  wouldn't have worked, so now it just reverts to the old inefficient code for cases that cross zero.

Finally, the new patch shows the warning each time instead of only the first time.  I still think it is useful to have a "once only" mode for warnings.  For example, if the code that does "if(array)" is third party code, the user will probably not want to modify it -- especially if it is updated frequently.  Still, it would be better to be part of a framework rather than a kludge for this particular warning.

(file #37563)

Lachlan Andrew <lachlan>In charge of this item.
Thu 23 Jun 2016 03:04:05 PM UTC, comment #10: 

I would remove the code the code that prevents the warning being issued multiple times between keyboard prompts.  It complicates the code and increases the maintenance burden.  If someone runs an m-file with the "if (array)" construct it will issue a few warnings, maybe a lot if it is in a for loop.  At that point either the user changes the code or turns off the warning.  Since the keyboard prompt code is helpful for at most one design iteration I think it can be removed.

I'd like to understand why 'test XXX' works but 'make check' doesn't.  That probably means some other problem to debug, but I find the longer form syntax suggested by Colin easier to understand and would prefer to use it.

Rik <rik5>
Project Administrator
Thu 23 Jun 2016 02:15:41 PM UTC, comment #9: 

JWE, I'm happy to remove the tolerance.  I figured it would be easier to remove than to add, so I put it in before seeking feedback.

The attached version ignores roundoff in ranges, so it matches "all (r != 0)".  It also makes first_array_as_logical_since_keyboard static, with a global reset function.

(file #37560)

Lachlan Andrew <lachlan>In charge of this item.
Thu 23 Jun 2016 06:14:44 AM UTC, comment #8: 

Ah yes.  Reading the original bug report.  That would help...
I've added some tests for pure-imaginary cases, returning Octave's actual behaviour.  I don't know if it is documented, but it seems reasonable behaviour.  However, I'm happy to change it if someone with more authority says to.

The funny format is valid ("warning" is a function that returns a value).  The syntax you suggested works fine for "test errwarn.cc-tst" from within Octave, but fails (reports no error) for "make check".

I've also fixed up octave_value::is_equal() to call all() explicitly so that the warning isn't thrown by, for example, switch statements.

Lachlan Andrew <lachlan>In charge of this item.
Thu 23 Jun 2016 05:39:04 AM UTC, comment #7: 

Warning once might be OK, but I don't like the addition of the global variable first_array_as_logical_since_keyboard.

I haven't tried to cook up a test case that fails, but can we be sure that the new test in Range::is_true won't result in bug reports about  how "all (r != 0)" disagrees with the is_true test?

John W. Eaton <jwe>
Project Administrator
Thu 23 Jun 2016 04:42:52 AM UTC, comment #6: 

Those look good to me.

Is this syntax correct, specifically the "+"?

%!error (warning ("error", "Octave:array-as-logical") + ([1 1] || 0));

I expected something like:

%!error <boolean value implies all>
%! warning ("error", "Octave:array-as-logical")
%! q = ([1 1] || 0);

My original comment mentions some cases like "if([1i 2i])".  Do you think we should file a separate issue for those or add tests here?

Colin Macdonald <cbm>
Thu 23 Jun 2016 03:33:20 AM UTC, comment #5: 

This new patch has some tests.  Were there any others you had in mind, Colin?

(file #37554)

Lachlan Andrew <lachlan>In charge of this item.
Mon 13 Jun 2016 07:38:58 PM UTC, comment #4: 

I personally like the idea of showing warnings once (or at least once).

I haven't tested your patch but I notice it does not add any tests.  I think there should be some new tests.

Colin Macdonald <cbm>
Sun 12 Jun 2016 08:14:25 AM UTC, comment #3: 

Here is a patch that seem to work.

Rik, rather than making this disabled by default, is it sufficient that it just prints the warning once per invocation, as it currently does?

I plan to write a patch soon that will make each warning default to outputting the command to disable it, like Matlab does.  (It will use the currently-unused "verbose" mode of warnings.)  That will allow those who don't want the warning to turn it off easily.  Otherwise, people who do want the warning will never learn of its existence, unless they read "warning_ids" every time they upgrade.

I've added the sparseness issue to the subject and increased the severity to "important".  Let me know if I should file a separate bug report instead.  The patch fixes both issues.

(file #37454)

Lachlan Andrew <lachlan>In charge of this item.
Sat 11 Jun 2016 11:55:06 AM UTC, comment #2: 

I'm looking into this, and noticed that most of the "special" types (diagonal, sparse etc.) have quite inefficient implementations of is_true().

More seriously,

octave:1> if ([NaN, 1; 1, NaN]), 1, else 0, end
error: invalid conversion from NaN to logical
octave:1> if sparse([NaN, 1; 1, NaN]), 1, else 0, end
ans = 1

Also, I'd like to have the warning issued at most once between each pair of interactive keyboard events.  Is there a standard mechanism for that?  Currently I just have a flag that gets set each time interactive_input() is called.

Lachlan Andrew <lachlan>In charge of this item.
Wed 25 Nov 2015 01:24:19 AM UTC, comment #1: 

I'm changing the title to reflect the first issue which is that the syntax "if (array)" should raise a warning so that the programmer has an idea that they may have written something badly.  The warning should be configurable and default to "off" since a lot of code has been written to expect this behavior.

Rik <rik5>
Project Administrator
Thu 28 Aug 2014 09:32:19 AM UTC, original submission:  

Personally, I think this should raise a warning, telling people we are defaulting "if (all(array))".  IMHO, "if (array)" is almost certainly a bug in the user's code (she expected a scalar).

Nonetheless that is not what Matlab does.  What they do is contradictory to their docs.  What their docs say is weird.

%% two easy cases

a = [1 1 1];
if (a), disp('got it right'); end
if (a), assert(true); else, assert(false); end

a = [1 0 1];
if (a), disp('got it wrong'); else, disp('got it right'); end
if (a), assert(false); else, assert(true); end

%% now the harder ones

a = [1 0+1i 1];
all(a)
if (a), disp('true??'); else, disp('false??'); end

a = [1 1+1i 1];
all(a)
if (a), disp('true, good'); else, disp('false, bad'); end

a = [1i 2i];
all(a)
if (a), disp('true??'); else, disp('false??'); end

%% Matlab:
% all of these give:
% error <Complex values cannot be converted to logicals>
% This contradicts their documentation as "help if" says:
%   > The statements are executed if the real part of the
%   > expression has all non-zero elements.
% (tested on 2008a and 2014a)

%% Octave:
% evaluates to true.  Is this documented?

Please don't change it to Matlab's documented behaviour :)

Colin Macdonald <cbm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #37625:  if_array.diff added by rik5 (4KiB - application/x-download)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by cbm (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-29 rik5 Attached File- => Added if_array.diff, #37625
        StatusPatch Submitted => Fixed
        Open/ClosedOpen => Closed
    2016-06-25 lachlan Attached File- => Added bug_43098_warn_if_array.v5.cset, #37574
    2016-06-24 lachlan Attached File- => Added bug_43098_warn_if_array.v4.cset, #37563
    2016-06-23 lachlan Attached File- => Added bug_43098_warn_if_array.v3.cset, #37560
    2016-06-23 lachlan Attached File- => Added bug_43098_warn_if_array.v2.cset, #37554
    2016-06-12 lachlan Attached File- => Added bug_43098_warn_if_array.cset, #37454
        Severity1 - Wish => 4 - Important
        StatusConfirmed => Patch Submitted
        Assigned toNone => lachlan
        Summary"if (array)" should raise a warning => "if (array)" should raise a warning, and give the same result for dense and sparse
    2015-11-25 rik5 Severity3 - Normal => 1 - Wish
        Item GroupMatlab Compatibility => Feature Request
        StatusNone => Confirmed
        Summary"if (array)" inconsistent with matlab (which is inconsistent with itself) => "if (array)" should raise a warning

    Back to the top


    Powered by Savane 3.5