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.
|
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)
|
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)
|
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.
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.
|
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:
(but maybe its not a big deal).
|
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)
|
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.
|
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)
|
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.
|
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?
|
Thu 23 Jun 2016 04:42:52 AM UTC, comment #6:
Those look good to me.
Is this syntax correct, specifically the "+"?
I expected something like:
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?
|
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)
|
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.
|
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)
|
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,
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.
|
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.
|
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.
Please don't change it to Matlab's documented behaviour :)
|