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


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

Submitted by:  None
Submitted on:  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

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)

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:

Nicholas Jankowski <nrjank>
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

      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>
Project 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>
Project 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>
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>
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>
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.")

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

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>
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>
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>
Fri 30 Apr 2021 04:22:21 PM UTC, comment #1: 

comparing for compatibility what happens with Matlab 2021a:


data =


>> u = unique(data)

u =


>> 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>
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

octave:32> u = unique(data)

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



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

Attach Files:

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 rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)

    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.




    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.6