bugGNU Octave - Bugs: bug #60482, hist() misses error for...

 
 

bug #60482: hist() misses error for non-numeric input

Submitter:  None
Submitted:  Thu 29 Apr 2021 09:02:29 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Missed Error or Warning
Status:  Need Info Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Open Release:  * 6.2.0
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 30 Mar 2022 01:08:05 AM UTC, comment #12: 

Here is a patch for this old report:


diff -r 1bddc0ddea74 scripts/plot/draw/hist.m
--- a/scripts/plot/draw/hist.m  Tue Mar 29 16:15:21 2022 -0700
+++ b/scripts/plot/draw/hist.m  Tue Mar 29 21:02:20 2022 -0400
@@ -122,6 +122,15 @@

   ## Process possible second argument
   if (nargin == 1 || ischar (varargin{iarg}))
+    ## Require numeric bins if output argument is specified
+    tmp = varargin{iarg}
+    if (nargout >= 1          # output specified
+        && nargin == 2        # this check will not apply for more than 2 input args
+        && ! isnumeric (tmp)) # likely a character input
+          error ("hist: second argument must be numeric when seeking return value");
+    end
+    ## If here, no major problem, so continue with default bins
+
     n = 10;
     ## Use integer range values and perform division last to preserve
     ## accuracy.


New behavior:

octave:1> data = "ABBCCCDDDDEEEEE", u = unique(data)
data = ABBCCCDDDDEEEEE
u = ABCDE

octave:2> freq = hist (data, u)
tmp = ABCDE
error: hist: second argument must be numeric when seeking return value
error: called from
    hist at line 130 column 11

octave:3> freq = hist (data+0, u)
tmp = ABCDE
error: hist: second argument must be numeric when seeking return value
error: called from
    hist at line 130 column 11

octave:4> freq = hist (data+0, u+0)
freq =

   1   2   3   4   5


Arun Giridhar <arungiridhar>
Group Member
Mon 03 May 2021 05:10:28 PM UTC, comment #11: 

good point, since there's absolutely no use for char inputs if an output is called for, and it seems to handle well if they aren't and it's plotting instead.

conversation created here:
https://octave.discourse.group/t/question-about-hist-c-behavior-for-processing-char-inputs/1110

Nicholas Jankowski <nrjank>
Group Member
Mon 03 May 2021 04:39:36 PM UTC, comment #10: 

Second question depends on the first to some extent.  If we accept "char" data it would also seem consistent that we accept "char" bin centers.

However, if we do that I think the input validation becomes very hard.  If I use 'N' for numeric and 'C' for char then these two calling forms are fairly indistinguishable.


hist (Y, X, NORM)
      N, C, N

hist (Y, PROPERTY, VALUE)
      N, C       , N


On the other hand, if bin centers can only be numeric then the first form is NNN and can be distinguished from the second.

Rik <rik5>
Group administrator
Mon 03 May 2021 04:28:03 PM UTC, comment #9: 

First question, should Octave emit an error for "char" data?  Or should it just convert the data to numeric and continue?  Octave already behaves that way for logical data where it converts true/false to 1/0 automatically.

For reference, Matlab converts logical but gives an error for "char" data.  We could choose to be Matlab-compatible here, but I don't think we have to be.  Octave is a superset of Matlab and if it does more than Matlab that is okay.

Could ask this question over on maintainer's section of Octave Discourse if we want more confirmation.

Rik <rik5>
Group administrator
Sat 01 May 2021 04:40:15 AM UTC, comment #8: 

if our graphics obects were classes, it would be trivial to check if the string is a property of that class.  Otherwise, I don't know if there's a way to get a list of properties without actually making that graphics object. with that list a simple strcmp against the list would do it.

Nicholas Jankowski <nrjank>
Group Member
Sat 01 May 2021 03:44:40 AM UTC, comment #7: 

sorry, i misread see what you're saying. change the behavior so that rather than ignore trailing string inputs, if you're not plotting the histogram, throw an error if trailing string inputs are given at all, since they aren't used.

It's reasonable, but I'm not certain how receptive people would be to that sort of change.

Nicholas Jankowski <nrjank>
Group Member
Sat 01 May 2021 03:00:56 AM UTC, comment #6: 

except that you're allowed to call hist with only one numerical argument. if you don't pass it an input about bins, it assumes there are 10 equally spaced ones. this is necessary for compatibility.  so the second argument must be allowed to be numeric or char.

Nicholas Jankowski <nrjank>
Group Member
Sat 01 May 2021 01:57:28 AM UTC, comment #5: 

Please comment on this conceptual approach:


if (nargout > 1) # output argument specified
  if (nargin == 2) # should be only data and bins
    if ( !isnumeric (arg2)) # invalid data type
      error ("second argument should be numeric when seeking a return value.")
    end
  end
end


Hopefully this can avoid having to check against a big list of keywords, which would need to be maintained with a lot of effort.

Anonymous
Fri 30 Apr 2021 05:41:49 PM UTC, comment #4: 

for my second request in the last comment, i submitted a low-priority wishlist bug #60487.

since nothing parses that char string until set, I think the only way to catch this would be for hist or bar to check that first string against every possible valid character input for that space. the latest dev version of hist mentions that 'the full list of properties is documented at @ref{Patch Properties}'.  I currently see about 50 properties on that list [1]. 

Set also allows partial property name matching when it's unambiguous, so 'facenormalsmode' and 'facenormalsm' would be valid, but not 'facenormal' since that could be either of two properties. for hist to check string validity then, it would have to compare with every valid property and variation thereof. 

Don't really see a way forward here with our current version of hist accepting graphics properties, short of an effort to come up with a 'check_valid_property' function that could be called in places like this and kept up to date on its own.

[1] https://octave.org/doc/latest/Patch-Properties.html

Nicholas Jankowski <nrjank>
Group Member
Fri 30 Apr 2021 05:07:10 PM UTC, comment #3: 


looking through hist some more,  apparently hist itself just ignores anything non-numeric for the purpose of the calculation.  if no outputs, it passes everything else as parameters to bar for plotting.  If there are outputs (like your example) then it never even looks at the options to consider throwing an error.


>> freq = hist (data + 0, u)
freq =

   13    7    8    7   12   10   10    3   10   12

>> freq = hist (data + 0, u,'invalidparameter')
freq =

   13    7    8    7   12   10   10    3   10   12

>> hist (data + 0, u)
error: set: unknown property
error: called from
    __bar__>bars at line 349 column 10
    __bar__ at line 209 column 12
    bar at line 124 column 18
    hist at line 237 column 5


so, 1 - it seems there would have be a way for hist to check the 2nd or 3rd input for 'validity', as they're the only two hist itself parses. 2 - it would be useful for hist to actually name the property it has a problem with in the error above.

Nicholas Jankowski <nrjank>
Group Member
Fri 30 Apr 2021 04:47:18 PM UTC, comment #2: 

looking at hist, it allows a char input for bins because the input checker assumes that if the second input is class(char), it must be an option or property.  Hence, when hist isn't told how many bins to use, it just uses 10.

note that the output is the same as if no bin option had been speficied:


>> freq = hist (data + 0, u)
freq =

   13    7    8    7   12   10   10    3   10   12

>> freq = hist (data + 0)
freq =

   13    7    8    7   12   10   10    3   10   12


Now, why it doesn't throw an error on an unrecognized option is something else to peek at.  matlab's hist does not accept character options/properties as inputs, so can easily throw an error for any non-numeric for 2nd input or later. Octave appears to accept a large number of properties, so it's not quite that simple. 

probably easiest bugfix is to have it recognize the invalid option/parameter string and throw an error.

Nicholas Jankowski <nrjank>
Group Member
Fri 30 Apr 2021 04:22:21 PM UTC, comment #1: 

comparing for compatibility what happens with Matlab 2021a:


>> data = 'AABBBBBCCCCCCDDDDDDEFFFFFGGGHHHHHHHJJJJJJKKLLLLMMMMMMMMNNOOOPPPPPPPRRRSSSSSSSSTTUVVVWWWWWWWW'

data =

    'AABBBBBCCCCCCDDDDDDEFFFFFGGGHHHHHHHJJJJJJKKLLLLMMMMMMMMNNOOOPPPPPPPRRRSSSSSSSSTTUVVVWWWWWWWW'

>> u = unique(data)

u =

    'ABCDEFGHJKLMNOPRSTUVW'

>> freq = hist (dada, u)
Unrecognized function or variable 'dada'.

Did you mean:
>> freq = hist (data, u)
Error using hist (line 75)
Input arguments must be numeric or a categorical array.

>> freq = hist (data + 0, u)
Error using hist (line 75)
Input arguments must be numeric or a categorical array.

>> freq = hist (data+0, u+0)

freq =

     2     5     6     6     1     5     3     7     6     2     4     8     2     3     7     3     8     2     1     3     8


Nicholas Jankowski <nrjank>
Group Member
Thu 29 Apr 2021 09:02:29 PM UTC, original submission:  

The hist() function gives an error for non-numeric data but not for non-numeric bins. Either it should accept non-numeric bins and return the correct frequency count, or it should give an error for non-numeric bins.


octave:31> data
data = AABBBBBCCCCCCDDDDDDEFFFFFGGGHHHHHHHJJJJJJKKLLLLMMMMMMMMNNOOOPPPPPPPRRRSSSSSSSSTTUVVVWWWWWWWW

octave:32> u = unique(data)
u = ABCDEFGHJKLMNOPRSTUVW

octave:33> freq = hist (data, u)          # was trying to get a histogram of letters
error: isfinite: argument must be numeric
error: called from
    hist at line 99 column 11

octave:34> freq = hist (data+0, u)        # this was accidental, but the result is very surprising, hence this bug report
freq =

   13    7    8    7   12   10   10    3   10   12

octave:36> freq = hist (data+0, u+0)      # this was the correct command to use and the correct result
freq =

   2   5   6   6   1   5   3   7   6   2   4   8   2   3   7   3   8   2   1   3   8


Anonymous

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-05-03 rik5 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code