bugGNU Octave - Bugs: bug #49793, octave's inputParser only accepts...

 
 

bug #49793: octave's inputParser only accepts validation functions that return true or false

Submitter:  None
Submitted:  Wed 07 Dec 2016 01:46:50 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.0
Operating System:  * GNU/Linux Fixed Release:  9.1.0
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 15 Apr 2023 02:19:45 PM UTC, comment #11: 

I have a solution for this which bypasses the unimplemented nargout functionality and does not rely on nested try/catch blocks which are a performance hit and a likely frequent occurrence given how validateattributes() works.

Relevant part of the patch is shown below:


      ## Validation function can either produce a true/false result or have
      ## no outputs but throw an error when failing.  Tricky code here
      ## relies on side effect, but calls validation function only once
      ## which is a performance win and may also be necessary if validation
      ## function maintains state.  See bug #49793.
      err = sprintf ('Checked with "%s"', func2str (val));
      ans = true;
      try
        val (in);  # call function with no arguments in case nargout == 0
        ok = ans;  # use side effect of assignment to 'ans' when nargout == 1
      catch exception
        ok = false;
        err = exception.message;
      end_try_catch


I checked the changeset in on the development branch here: http://hg.savannah.gnu.org/hgweb/octave/rev/5945e1bd73ea.  This will be a part of Octave 9.1 released in 2024.  Because this is just an m-file change you can always go to Mercurial version control system, download the m-file, and replace your local copy of inputParser.m to get the change earlier.

Marking bug as Fixed and closing report.

Rik <rik5>
Group administrator
Fri 20 Oct 2017 12:50:43 PM UTC, comment #10: 

I stumbled on the same problem as described in this bug when using the function dirPlus.m from Matlabs FileExchange. There the function validateattributes() was used for argument checking in input parsers. This function has no return value and does throw an error if argument checking fails. In matlab it seems to be valid to do so.

I have created a fix for inputParser in octave to handle this type of functions that does also print a bit more verbose information on what has been checked if the check fails.

See my fix for inputParser.m line ~500 below.


function validate_arg (this, name, val, in)
 % Check validation result to be empty in case of the use of validateattributes function
 % An empty result will be interpreted as valid arguments
 check = {val(in)};
 if (~isempty(check) && ~val(in))
   this.error (sprintf ("failed validation of %s with %s", toupper (name),func2str(val)));
 endif
 this.Results.(name) = in;
 endfunction


Take care: in the original source "val (in)" was written with a blank. This leads to a different result in this statement. It must be written without blank as "val(in)"!

Georg Wiora <gwiora>
Thu 08 Dec 2016 07:47:15 PM UTC, comment #9: 

An anonymous function is similar to a regular one defined as: "function varargout = fcn(x)", and nargout will return -1 for both, just like MATLAB. Hence I was checking "nout~=0" rather than "nout>0".

But you're right, my version won't work unless there was a fixed error ID/message for the too-many-outputs case. Not to mention I assumed it was okay to call the validation twice..

I don't know how hard it is, but I guess a fix for nargout is needed here.

Amro <amro_octave>
Thu 08 Dec 2016 06:27:00 PM UTC, comment #8: 

The main point of jwe's idea was that it does depend on fixing nargout so it works for builtins. I wonder about anonymous functions though.

Amro - a couple points about your suggestion. There is no error ID that can be caught in Octave yet for the error of too many output arguments (that can be fixed of course). And this nested approach means that val may be called more than once on the given argument. It's likely that most validation routines will be pure functions, but it's possible to have some side effects from calling the validation function more than once.

Mike Miller <mtmiller>
Group Member
Thu 08 Dec 2016 04:19:28 PM UTC, comment #7: 

@jwe
if nargout doesn't work with builtins, then calling something like
  ip.addRequired('name',@isnumeric);
will always fail in your solution because "nargout" will throw an error...

I think we need to need to test the error in the catch block and attempt calling the validation function another time inside a second try/catch if the error indicated a too-many-output case.

How about this version (it's sort of a hack until "nargout" is fixed):


try
    nout = nargout(val);
catch
    nout = -1;  % val is a builtin function
end

result = true;
try
    if nout ~= 0
        result = val(in);
    else
        val(in);
    end
catch ME
    if strcmp(ME.identifier, 'Octave:TooManyOutputs')
        try
            val(in);
        catch
            result = false;
        end
    else
        result = false;
    end
end


Amro <amro_octave>
Thu 08 Dec 2016 03:33:40 PM UTC, comment #6: 

After thinking about this some more, I agree that the best solution is to do use nargout to check to see whether the function returns at least one output and use a try-catch block.  So it would be something like this:


result = true;
try
  if (nargout (val) > 0)
    result = val (in);
  else
    val (in);
  endif
catch
  ## Some kind of error happened in calling the
  ## validation function, so consider it to have failed.
  result = false;
end_try_catch


Even though nargout doesn't work properly for all functions yet, this solution will work for user-defined validation functions.

We can also make it a priority for the next release to fix Octave so that nargout will work for built-in functions as well.

John W. Eaton <jwe>
Group administrator
Thu 08 Dec 2016 03:12:10 PM UTC, comment #5: 

nargout doesn't work for built-in functions yet.

That this works:


function f () end
f || false


might be seen as a bug (it is apparently not the same as what Matlab does) so someone might fix it and then your "fix" here won't work.

John W. Eaton <jwe>
Group administrator
Thu 08 Dec 2016 01:25:52 PM UTC, comment #4: 

perhaps a check to "nargout(val)" to differentiate between the two cases of validation functions, coupled with a try/catch block.

Amro <amro_octave>
Thu 08 Dec 2016 09:45:49 AM UTC, comment #3: 

As in octave a || b apparently returns the logical value of b if a is undefined (e.g. a is a call to a function which returns nothing), I suggest the following one-line-fix in inputParser.m:


function validate_arg (this, name, val, in)
        if (! val (in) || false )   % <- only this line changed!
          this.error (sprintf ("failed validation of %s", toupper (name)));
        endif
        this.Results.(name) = in;
    endfunction


The only drawback I see so far is, that 'failed validation of %s' is only printed if the validation function does not throw an error before. Here a hopefully comprehensive example, using inputParser.m patched as described above:


octave:1> htest = @(h) validateattributes(h, {'double'}, {'nonnegative'});
octave:2>
octave:2> ip = inputParser;
octave:3> ip.addRequired('h', htest);
octave:4> ip.parse(1.0);
octave:5> ip.parse(-1.0);
error: input must be nonnegative
error: called from
    validateattributes at line 406 column 7
    validate_arg at line 505 column 9
    parse at line 394 column 9
octave:5>
octave:5> function ret = myvalidatenonnegative(a)
> if a >= 0
> ret = true;
> else
> ret = false;
> endif
> endfunction
octave:6>
octave:6> Htest = @(h) myvalidatenonnegative(h)
Htest =

@(h) myvalidatenonnegative (h)

octave:7>
octave:7> ip = inputParser;
octave:8> ip.addRequired('h', Htest);
octave:9> ip.parse(1.0);
octave:10> ip.parse(-1.0);
error: failed validation of H
error: called from
    error at line 539 column 7
    validate_arg at line 506 column 11
    parse at line 394 column 9
octave:10>


Anonymous
Wed 07 Dec 2016 03:53:02 PM UTC, comment #2: 

It would also be good idea to add a "nargoutchk" check to validateattributes, that way it produces a better error message saying 'Too many output arguments'.

Amro <amro_octave>
Wed 07 Dec 2016 03:46:16 PM UTC, comment #1: 

+1 I came across the same error as well.

Amro <amro_octave>
Wed 07 Dec 2016 01:46:50 PM UTC, original submission:  



ip = inputParser;
htest = @(h) validateattributes(h, {'double'}, {'nonnegative'});
ip.addRequired('h', htest);
ip.parse(1.0);


fails with


error: if: undefined value used in conditional expression
error: called from
    validate_arg at line 505 column 9
    parse at line 394 column 9


From Matlab's documentation (cf. https://de.mathworks.com/help/matlab/ref/inputparser.addrequired.html):
"inputParser accepts two types of validation functions: functions that return true or false, and functions that pass or throw an error. Both types of validation functions must accept a single input argument."


octave/4.2.0/m/general/inputParser.m lines 504 to 509:

    function validate_arg (this, name, val, in)
        if (! val (in))    % <- octave throws here the error
          this.error (sprintf ("failed validation of %s", toupper (name)));
        endif
        this.Results.(name) = in;
    endfunction


Octave's function validate_arg which is called from Octave's inputParser's parse function can handle only validation functions that return true or false.
It has to be rewritten, to cope with validation functions that return nothing or throw and error, as validateattributes does, to be MATLAB compatible.

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 rik5 (Posted a comment)
  • -email is unavailable- added by gwiora (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by amro_octave (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-04-15 rik5 StatusNone Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code