bugGNU Octave - Bugs: bug #67199, roots() permits non-numeric input...

 
 

bug #67199: roots() permits non-numeric input types

Submitter:  chad <chadholton>
Submitted:  Sun 08 Jun 2025 01:13:31 PM UTC
   
 
Category:  Octave Function Severity:  2 - Minor
Priority:  3 - Low Item Group:  Missed Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  chad Open/Closed:  * Closed
Release:  * 10.2.0 Release: 
Operating System:  * Any Fixed Release:  11.1.0 (current default)
Planned Release:  11.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 17 Jun 2025 04:35:18 AM UTC, comment #13: 

I checked in stricter input validation along with new BIST tests here: https://hg.savannah.gnu.org/hgweb/octave/rev/dfb46a2d2249.

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Thu 12 Jun 2025 05:49:30 PM UTC, comment #12: 

I think it is useful that Octave promotes integer types to double and proceeds with calculation.  I think using a character vector as input is likely to be a mistake.  For that reason, I would support adding a test for isnumeric() to the input validation of roots().  If the user really wants the roots of a string then they can easily convert using double() and be explicit about their intentions.

roots ('string')
=>
roots (double ('string'))


Rik <rik5>
Group administrator
Tue 10 Jun 2025 07:27:38 AM UTC, comment #11: 

In Matlab R2025a:

>> roots('test')
Error using zeros
Argument following "like" must be a variable with a data type that supports ZEROS, such as a double or single variable.

Error in roots (line 40)
fullPrototype = full(zeros("like",c));
                     ^^^^^^^^^^^^^^^
Related documentation

>>


So, it looks like not supporting char vectors as input of `roots` isn't an explicit choice in Matlab. (It looks more like an "implementation accident".)

While we usually try to be Matlab compatible where this is reasonable, we don't try to be bug-for-bug compatible.

Imho, the current behavior of Octave is a nice extension that shouldn't cause compatibility issues.

What do others think?

Markus Mützel <mmuetzel>
Group administrator
Mon 09 Jun 2025 06:25:12 PM UTC, comment #10: 

sorry, i tested that when making comment #8 but didn't include it.


>> roots(int16([1 0 1]))
Error using eig
Invalid data type. Input matrix must be double or single.

>> roots(logical([1 0 1]))
Error using eig
Invalid data type. Input matrix must be double or single.


so the error is also related to a call to eig needing a float, similar to what we sometimes see for other types.

Nicholas Jankowski <nrjank>
Group Member
Mon 09 Jun 2025 06:01:07 PM UTC, comment #9: 

According to Matlab documentation, the acceptable classes for the input to roots() are single or double.  However, can you test in Matlab whether this is true?  My guess is they might automatically promote integer and char vector inputs to double.  This is what Octave does.


octave:1> y = int16 ([1 0 1])
y =

  1  0  1

octave:2> roots (y)
ans =

   0 + 1i
   0 - 1i



Rik <rik5>
Group administrator
Mon 09 Jun 2025 03:58:35 PM UTC, comment #8: 

in answering my last question, Octave also permits int/logical/etc. inputs. pretty much anything that doesn't cause an error to be thrown and can be processed as numeric by any of the called numeric functions (eig throws "isfinite not defined for ..." errors for cells and structs) is fine.

Matlab throws an 'input matrix must be double or single' error for any of those.

The output is correct for the inputs given (try wrapping any inputs in double() and you get the same results in octave or matlab.)

is it necessary to add to the input validation here? roots gets called by a few other functions so may need do stay lightweight?

Nicholas Jankowski <nrjank>
Group Member
Mon 09 Jun 2025 03:45:05 PM UTC, comment #7: 

 so circling back to the original issue, Octave's roots permits char inputs to be interpreted as their numerical equivalent, while Matlab throws an error.

it would be very simple to add an isnumeric or isfloat (haven't checked ints and logicals) to limit that if it's desired.

Would that limit any legitimate use cases?

Nicholas Jankowski <nrjank>
Group Member
Mon 09 Jun 2025 03:39:00 PM UTC, comment #6: 

and checking again, allfinite also permits char inputs, it only errors on string objects:


>> allfinite('test')

ans =

  logical

   1

>> allfinite("test")
Incorrect number or types of inputs or outputs for function isfinite.


Nicholas Jankowski <nrjank>
Group Member
Mon 09 Jun 2025 03:37:28 PM UTC, comment #5: 

1) octave's isfinite was written to be compatible with Matlab's isfinite function. allfinite has a different use and purpose.  i haven't verified but from the documentation, the primary functional behavior of `allfinite(x)` appears to be the same as `all(isfinite(x)(:))`.  it seems implementing an Octave wrapper for that would be trivial, and we'd appreciate any code contributions (following Octave's contribution guidelines of course.)

2) matlab's isfinite also permits char arrays (but it does throw an error for string object inputs, as the two are no longer the same in Matlab).  so changing isfinite would introduce a matlab incompatibility.

3) please review the following: https://wiki.octave.org/Contribute#Contributor_Agreement , specifically the second bullet regarding matlab code.

Nicholas Jankowski <nrjank>
Group Member
Mon 09 Jun 2025 03:16:42 PM UTC, comment #4: 

I mean initially I thought that just adding the !isnumeric(c) restriction to ensure that the input was of numeric type could solve the problem.

However, after comparing the MATLAB code, I realized that the input checking of the two roots() functions is similar, but the reason for the different results is due to the difference in the functionality of the two functions, Octave's isfinite() and MATLAB's allfinite(). Octave's isfinite() allows string input, which leads to this bug67199.

Adding !isnumeric(c) solves the problem. Or is it possible to modify isfinite() to be compatible with MATLAB's allfinite()?

chad <chadholton>
Mon 09 Jun 2025 02:33:52 PM UTC, comment #3: 

Please DO NOT examine Matlab internals or base any Octave contributions on MathWorks code that you may have access to view.

John W. Eaton <jwe>
Group administrator
Mon 09 Jun 2025 01:40:02 PM UTC, comment #2: 

After testing, I have a new discovery.The input checking of roots() in MATLAB and Octave is similar.

Octave
 if (nargin < 1 || (! isvector (c) && ! isempty (c)))
    print_usage ();
  elseif (any (! isfinite (c)))
    error ("roots: inputs must not contain Inf or NaN");
  endif

MATLAB
if ~isempty(c) && ~isvector(c)
    error(message('MATLAB:roots:NonVectorInput'));
end

if ~allfinite(c)
    error(message('MATLAB:roots:NonFiniteInput'));
end

But there is a difference between Octave's isfinite() and MATLAB's allfinite().Octave's isfinite() accepts string input.

Octave
 >> isfinite("string")
ans =
  1  1  1  1  1  1
>> isfinite([1 "aaaa" 2])
warning: implicit conversion from numeric to char
ans =
  1  1  1  1  1  1

MATLAB
>> allfinite("string")
Incorrect number or types of inputs or outputs for function isfinite.

Error allfinite (line 11)
tf = all(isfinite(X), "all");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


chad <chadholton>
Sun 08 Jun 2025 01:32:52 PM UTC, comment #1: 


roots("string")
ans =

   0.4864 + 0.8456i
   0.4864 - 0.8456i
  -0.5140 + 0.8502i
  -0.5140 - 0.8502i
  -0.9535 +      0i

The ASCII codes for the corresponding letters in the string are [115, 116, 114, 105, 110, 103].

roots([115, 116, 114, 105, 110, 103])
ans =

   0.4864 + 0.8456i
   0.4864 - 0.8456i
  -0.5140 + 0.8502i
  -0.5140 - 0.8502i
  -0.9535 +      0i

The result is the same.

chad <chadholton>
Sun 08 Jun 2025 01:13:31 PM UTC, original submission:  

The roots() function is incomplete for input checking, resulting in unreasonable output.I can still get results with strings as input,such as belows.Octave interprets strings as arrays of ASCII values, and therefore can gets the root.

roots("string")
roots([1 "sa" 1])

This is also relatively easy to fix by changing the input check code at the beginning.
in line 87(roots.m)
From

if (nargin < 1 || (! isvector (c) && ! isempty (c)))

To

if (nargin < 1 || ! isnumeric (c) || (! isvector (c) && ! isempty (c)))


chad <chadholton>

 

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

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2025-06-17 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 11.1.0 (current default)
    2025-06-12 rik5 StatusNone In Progress
        Planned ReleaseNone 11.1.0 (current default)
    2025-06-12 rik5 Priority5 - Normal 3 - Low
    2025-06-12 rik5 Severity3 - Normal 2 - Minor
    2025-06-09 nrjank Release10.1.0 10.2.0
        Operating SystemMicrosoft Windows Any
        SummaryOctave function roots() is incomplete for input checking. roots() permits non-numeric input types
    2025-06-09 chadholton Carbon-CopyRemoved 392912 -

    Back to the top

    Powered by Savane 3.15-26b0.
    Corresponding source code