bugGNU Octave - Bugs: bug #48439, validateattributes throws errors...

 
 

bug #48439: validateattributes throws errors without IDs

Submitter:  Carlo de Falco <cdf>
Submitted:  Thu 07 Jul 2016 04:34:32 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Postponed Assigned to:  cdf
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 01 Oct 2016 03:37:27 PM UTC, comment #23: 

I think the issue of this discussion is that we are looking at the error
ids from the point of view of different users.  Who will be the user of
the error ids?  Given a function foo() that uses validateattributes for
input check, who will be using the error ids?  The developer of foo() or
the caller of foo()?

For the caller of foo(), the new error ids will not be useful.  The caller
of foo() will not even know which of the arguments failed validation and
so there's no point on knowing why that unknown argument failed validation.
In addition, with the new error ids, the caller of foo() will not even be able
to know if foo() failed because of invalid arguments (having the caller of
foo() list all possible 28 error ids related to invalid argument is not doable).

So the only user of the new error ids is the developer of foo().
But why should the developer of foo() care about them?  The only reason
to catch an exception is if there's some exception handling to do.
What exception handling is there to do for an invalid argument that
is dependent on knowing why the argument is invalid?  The only case
where I see that being used is if the argument is not really invalid
and exceptions are being used for flow control.

This questions are not rhetorical.  Can someone please explain the design
of this choice and why this change?

Also, I'm removing the Matlab compatibility tag since we are not Matlab
compatible on error ids and there are no plans to be so.

Carnë Draug <carandraug>
Group Member
Wed 28 Sep 2016 04:26:42 PM UTC, comment #22: 

After discussion in IRC with mtmx and jwe
I'm reverting http://hg.savannah.gnu.org/hgweb/octave/rev/5e27d8d4e3c1

I'll leave the discussion open, though, because the general
approach for choosing error IDs is not really set yet ...



(file #38615)

Carlo de Falco <cdf>
Group Member
Sun 14 Aug 2016 11:09:51 PM UTC, comment #21: 


> This does NOT seem to address the issues raised in the discussion.
>


Can you be more specific about the issues it does not address?

  • It adds an error id to the error message -- issue addressed.
  • It allows to distinguish failed input check (error id "invalid-input-arg" as already documented) from incorrect calling of validateattributes (error id "Octave:validateattributes:invalid-input-arg") -- issue addressed.
  • It only adds one new error id (which is unique to calling validateattributes incorrectly) -- issue of not adding 34 new error ids addressed.


> You just removed the specific error ids expressing the different reasons for failure.
>
> But you still introduced more new error IDs.
>
> So this does still increase the total number of error IDs in Octave, but to really little use.


I agree that the validateattributes unique error id is of little use.  I'm only adding it as compromise since that was one of the issues.  I'll gladly remove it and make it use the standard "Octave:invalid-input-arg" if you prefer.

I will mention this one more time: too many error ids in Octave makes error ids useless because Octave has no class system for errors.  No one will ever want to catch only the "expected-less-equal" and treat it different from a "expected-less-equal".  Even languages that do have a class system for errors do not go into such detail. See python https://docs.python.org/3/library/exceptions.html#exception-hierarchy (which has only TypeError and ValueError) and C++ http://en.cppreference.com/w/cpp/error/exception (which only has invalid_argument) for examples.

Carnë Draug <carandraug>
Group Member
Sun 14 Aug 2016 10:27:29 PM UTC, comment #20: 

This does NOT seem to address the issues raised in the discussion.

You just removed the specific error ids expressing the different
reasons for failure.

But you still introduced more new error IDs.

So this does still increase the total number of error IDs in Octave, but to really little use.

This seems like the worse possible choice to me.

I'd suggest you revert this change while we wait for comments
from others.
c.

Carlo de Falco <cdf>
Group Member
Sun 14 Aug 2016 04:47:35 PM UTC, comment #19: 

I pushed this http://hg.savannah.gnu.org/hgweb/octave/rev/5e27d8d4e3c1 which I think addresses everyone issues.

Carnë Draug <carandraug>
Group Member
Tue 26 Jul 2016 04:20:59 PM UTC, comment #18: 

Carlo de Falco on comment #17:

> [...] and I don't see your arguments as a good enough reason for Octave to deviate from Matlab compatibility here.


There is no Matlab compatibility to keep because our error are Octave:X instead of Matlab:X. Also, Matlab error ids are now Matlab:FunctionName:invalidX

From Matlab docs:

> The function name becomes part of the error identifier.
>
> MException.last.identifier
>
> ans =
>
> MATLAB:findVolume:invalidType


Lachlan on comment #1:

> Carlo, it would be nice if the error ID for a faulty call to this function was different from a valid call that stated that the attribute is not valid.
>
> You don't need to limit yourself to the existing error IDs, do you?


Kind of. Since this function can be used by octave core functions to validate their input, then it needs to be limited to Octave core error ids.  But that list can be increased.

The error id refers to what caused an error with the most detailed error id documented.  What caused an error was an invalid argument and that's independent of whether that invalid argument was for validateattributes or some other function.  Adding more details to the error (expected-1d, expected-string, etc) as proposed does not fix the issue because if those error id are added, then they should also be used by validateattributes itself.

If this is really necessary, then it should be done the other way around, i.e., have validateattributes throw its own specific error id if called incorrectly. Something like "octave:validateattributes-incorrect-usage".  It addresses the issue of being able to distinguish the two causes and doesn't reduce the usefulness of the error ids.

However, I'm not really convinced that will be necessary. error ids are used when handling the error programatically (otherwise one would use the error message where the function name is mentioned). I can't imagine anyone programming error handling for validateattributes incorrect usage.

Carlo on comment #2

> The error is an invalid input argument in both cases, and the error message does specify whether the invalid arg was passed to validateattributes itself or to a function using validateattributes internally, so I think that part is not that bad ...


Agreed

> [...]
> I am not limited to existing IDs but I remember previous discussions about not introducing too many different IDs and this would probably introduce a lot.


You're right. This has been discussed before. Let's not introduce loads of new error ids. If there's too many without an exception subclass they become useless.

Carnë Draug <carandraug>
Group Member
Tue 26 Jul 2016 08:58:02 AM UTC, comment #17: 

Carnë, yes of course, I was just too busy to do so right now,
and wanted to make sure the issue is not considered closed until I have time to write a longer answer.

I'll come back to this ASAP.

Anyway, in short, I find arguments from Rik and Lachlan more convincing than yours, and I don't see your arguments as a good enough reason for Octave to deviate from Matlab compatibility here.




Carlo de Falco <cdf>
Group Member
Sun 24 Jul 2016 07:06:44 PM UTC, comment #16: 

Carlo, could you add more details to that?

Carnë Draug <carandraug>
Group Member
Sun 24 Jul 2016 03:37:28 PM UTC, comment #15: 

I oppose

Carlo de Falco <cdf>
Group Member
Fri 22 Jul 2016 04:47:32 PM UTC, comment #14: 

If no one opposes, I will then undo the commit that adds the detailed warning ids.

Carnë Draug <carandraug>
Group Member
Wed 13 Jul 2016 04:55:22 PM UTC, comment #13: 


> I think I would use validateattributes and let the backtrace that gets dumped automatically show that the error was generated in validateattributes, but the calling function was my_function.m. Isn't that how Matlab handles it?


Yes. But my point is that if core functions use validateattributes for input check, and if core functions let validateattributes throw the error, then they will use the validateattributes error ids. Since that happens, then we should have validateattributes throw the error ids that we want the core functions to throw. So this is not about what error ids validateattributes should throw. This about what error ids Octave core should throw.

Also, if they are the error ids that core functions throw, then they should be documented in the Octave manual or in the help text for error ids (and not in validateattributes help text).

It all boils down to: do we want Octave core functions to throw error ids with this level of detail? Or do we want the error id to be just "invalid-input"?

I think we should only throw "invalid-input" for two reasons.

First reason is that there's no hierarchy in error ids, no class system (or at least not yet in Octave). We can't catch all errors that have id of "expected-X" by catching "invalid-input".  Because of that, it would be more useful to have them all throw the same error id. "invalid-input" sounds good to me, but I don't care much for another wording.

The other reason is I find error ids to not be that useful to handle programatically.  And if the error ids are only useful if they are read by a person, then why have them at all? we already have the error message for that, no need for the error id as well.  In this specific case, it is likely that input fails for multiple reasons, for example, wrong size and non-negative.  The error id is then not enough to explain all that went wrong.

Carnë Draug <carandraug>
Group Member
Tue 12 Jul 2016 08:07:38 PM UTC, comment #12: 

re. comment #11: yes, when validateattributes is used to catch an error within a function both the error message and the backtrace can be used to know what function was actually invoked with the wrong input argument

re. comment #8: actually the new ids ARE also being used to mark
the errors in invocation of validateattributes itself, when the error did not seem to easily fit one of the options I used the more generic "Octave:invalid-input-arg". Probably some cases where I used the generic IDs can be changed to use more specific identifier.

re. documentation: I cannot find a complete list of all error ids used in matlab in any page of matlab's documentation, do we really need one for Octave?

Carlo de Falco <cdf>
Group Member
Tue 12 Jul 2016 06:54:28 PM UTC, comment #11: 

I think I would use validateattributes and let the backtrace that gets dumped automatically show that the error was generated in validateattributes, but the calling function was my_function.m.  Isn't that how Matlab handles it?

Rik <rik5>
Group administrator
Tue 12 Jul 2016 06:45:21 PM UTC, comment #10: 


> If the function validateattributes is not called correctly, that is a general sort of error and can use the same general error set that is described in error_ids. For example, ode45 uses "Octave:invalid-input-arg" when it is called incorrectly.


So let's assume that the next m function I write uses validateattributes for input check? Should I catch the error and change it to invalid-inpu-arg? Or should I not use validateattributes in Octave?

Carnë Draug <carandraug>
Group Member
Tue 12 Jul 2016 06:31:32 PM UTC, comment #9: 

The error_ids list is more about general Octave errors.  For the time being I would document the specific errors that validateattributes throws in that function itself.

If the function validateattributes is not called correctly, that is a general sort of error and can use the same general error set that is described in error_ids.  For example, ode45 uses "Octave:invalid-input-arg" when it is called incorrectly.

Rik <rik5>
Group administrator
Tue 12 Jul 2016 06:07:19 PM UTC, comment #8: 

Asking for a review after you push is not really how this is supposed to work.

Anyway, this cset actually changes two things on the code:

  • error ids when validateattributes is called incorrectly.
  • error ids for the errors that validateattributes is supposed to thrown when called correctly.


Why are the ids for them different? If we "bless" the errors ids of the second set, then surely we should use them on the first set. In that case they are the same, and "Octave:invalid-invalid-type" should be changed to whatever it is that makes the input fail. I guess that validateattributes should use validateattributes.

Also, this should be definitely be documented somewhere.  Not sure where though. On the error_ids page and validateattributes making reference to it?

Carnë Draug <carandraug>
Group Member
Tue 12 Jul 2016 05:59:43 PM UTC, comment #7: 

OK, I applied both changes requested in comment #6
and added two tests for the "Octave:invalid-type"
error id with this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/d4b95a536e0b

if that looks OK I think we can close this issue.

Carlo de Falco <cdf>
Group Member
Tue 12 Jul 2016 04:01:58 PM UTC, comment #6: 

Rik here, I actually disclaim responsibility for the 4.2.0 release.  I'm very busy at work and then off for two weeks so I will help where I can, but can't manage the whole process this time.

As for the patch, it seems appropriate.

A couple questions, first


error ("Octave:invalid-invalid-type"


Should the word "invalid" really be repeated?  Maybe this would be better?


error ("Octave:invalid-type"


In the BIST tests, jwe prefers the use of double quotes to single quotes.  Could you do a search and replace from ' to ".  Also, Octave coding conventions are for a space between the function name and the opening parenthesis: "ones(1,2,3)" => "ones (1,2,3)".  It is slightly longer, but I think more readable, if you can use newlines and indentation to show the different sections of a try/catch block.  Summing up, I would change the first BIST test


%!test
%! try validateattributes (ones(1,2,3), {'numeric'}, {'2d'});
%! catch id,
%! assert (getfield (id, 'identifier'), 'Octave:expected-2d');
%! end_try_catch


to read as follows:


%!test
%! try
%!   validateattributes (ones (1,2,3), {"numeric"}, {"2d"});
%! catch id
%!   assert (getfield (id, "identifier"), "Octave:expected-2d");
%! end_try_catch



Rik <rik5>
Group administrator
Tue 12 Jul 2016 08:26:41 AM UTC, comment #5: 

Hi,

I just pushed this changeset:
http://hg.savannah.gnu.org/hgweb/octave/rev/6457ff946024

which sets error ids with codes mapped from matlab
with the procedure explained in comment #3

I'd appreciate feedback on this one.
Should I also document the new error ids in error_ids.m ?

Adding Carnë in CC as he was the original author of validateattributes and Rik as he is managing the 4.2.0 release.


Carlo de Falco <cdf>
Group Member
Tue 12 Jul 2016 07:23:17 AM UTC, comment #4: 

oops there was a typo in the previously posted script,
the corrected one is file #37822

the only changed output is


>> try validateattributes (NaN, {'numeric'}, {'nonnan'        }); catch id, disp (getfield(id,'identifier')); end
MATLAB:expectedNonNaN




Carlo de Falco <cdf>
Group Member
Tue 12 Jul 2016 07:19:19 AM UTC, comment #3: 

The new attachment file #37820 produces a list of all
error ids produced by validateattributes in Matlab when
the input argument does not satisfy a given condition,
the results are the following:


>> va_ids
MATLAB:expected2D
MATLAB:expected3D
MATLAB:expectedColumn
MATLAB:expectedRow
MATLAB:expectedScalar
MATLAB:expectedVector
MATLAB:incorrectSize
MATLAB:incorrectNumel
MATLAB:incorrectNumcols
MATLAB:incorrectNumrows
MATLAB:incorrectNumdims
MATLAB:expectedSquare
MATLAB:expectedDiag
MATLAB:expectedNonsparse
MATLAB:notGreater
MATLAB:notGreaterEqual
MATLAB:notLess
MATLAB:notLessEqual
MATLAB:expectedBinary
MATLAB:expectedEven
MATLAB:expectedOdd
MATLAB:expectedInteger
MATLAB:expectedReal
MATLAB:expectedFinite
MATLAB:UndefinedFunction
MATLAB:expectedNonnegative
MATLAB:expectedNonZero
MATLAB:expectedPositive
MATLAB:expectedDecreasing
MATLAB:expectedIncreasing
MATLAB:expectedNondecreasing
MATLAB:expectedNonincreasing


when the input argument does not match the expected type
the ID is always 'MATLAB:invalidType'

would it be OK to introduce all the same error IDS mapping
the names to Octave style as


MATLAB:expectedXxxx -> Octave:expected-xxxx



MATLAB:invalidType -> Octave:invalid-type


?

Should we also keep the generic error ID

"Octave:invalid-input-arg"

?




Carlo de Falco <cdf>
Group Member
Mon 11 Jul 2016 03:59:35 PM UTC, comment #2: 

Lachlan,

The error is an invalid input argument in both cases,
and the error message does specify whether the invalid arg
was passed to validateattributes itself or to a function using
validateattributes internally, so I think that part is not that bad ...

But MATLAB is even more specific, as it has a different identifier for each kind of test that validateattributes can perform on inputs, e.g. 'MATLAB:notLessEqual' if the input is required to be less or equal than a specified value but it isn't or 'MATLAB:invalidType' if it is not of one of the allowed types.

I am not limited to existing IDs but I remember previous discussions about not introducing too many different IDs and this would probably introduce a lot.


Carlo de Falco <cdf>
Group Member
Fri 08 Jul 2016 02:51:23 PM UTC, comment #1: 

Carlo, it would be nice if the error ID for a faulty call to this function was different from a valid call that stated that the attribute is not valid.

You don't need to limit yourself to the existing error IDs, do you?

Lachlan Andrew <lachlan>
Thu 07 Jul 2016 04:34:32 PM UTC, original submission:  

Hi,

When using validateattributes to check consistency of input
arguments, Matlab throws an error with a different identifier
depending on the type of error.

Octave does not currently set any identifier.

In Matlab :


>> validateattributes (1, {'numeric'}, {'<=', -1})
Expected input to be an array with all of the values <= -1.

>> lasterror

ans =

       message: 'Expected input to be an array with all of the values <= -1.'
    identifier: 'MATLAB:notLessEqual'
         stack: [0x1 struct]

>> validateattributes (rand (5), {'uint8'}, {})
Expected input to be one of these types:

uint8

Instead its type was double.

>> lasterror

ans =

       message: 'Expected input to be one of these types:…'
    identifier: 'MATLAB:invalidType'
         stack: [0x1 struct]


In Octave validateattributes does not currently set an ID
when throwing an error:



>> validateattributes (1, {'numeric'}, {'<=', -1})
error: input must be less than or equal to -1.000000
error: called from
    validateattributes at line 324 column 11
>> lasterror
ans =

  scalar structure containing the fields:

    message = input must be less than or equal to -1.000000
    identifier =

>> validateattributes (rand (5), {'uint8'}, {})
error: input must be of class:

  uint8

but was of class double
error: called from
    validateattributes at line 227 column 5
>> lasterror
ans =

  scalar structure containing the fields:

    message = input must be of class:

  uint8

but was of class double
    identifier =


The attached patch addresses this by adding an error ID.
The ID is always set to "Octave:invalid-input-arg" as it
seems the only appropriate one in the current list of error_ids.

Shall I push this chset?








Carlo de Falco <cdf>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #38615:  open_2hn7ueDF.txt added by cdf (18KiB - text/plain)
file #37822:  va_ids.m added by cdf (3KiB - text/x-objcsrc - test error messages in Matlab)
file #37760:  open_SyvI97zm.txt added by cdf (5KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by cdf
  • -email is unavailable- added by cdf
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by cdf (Submitted the item)
  •  

    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 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-24 mtmiller StatusFixed Postponed
    2016-10-01 carandraug Item GroupMatlab Compatibility None
    2016-09-28 cdf Attached File- Added open_2hn7ueDF.txt, #38615
    2016-08-14 cdf Open/ClosedClosed Open
    2016-08-14 carandraug StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-07-12 rik5 StatusPatch Submitted In Progress
    2016-07-12 cdf Carbon-Copy- Added -email is unavailable-
        Carbon-Copy- Added -email is unavailable-
    2016-07-12 cdf Attached File- Added va_ids.m, #37822
    2016-07-12 cdf Attached File#37820 Removed
    2016-07-12 cdf Attached File- Added va_ids.m, #37820
    2016-07-07 cdf Assigned toNone cdf
    2016-07-07 cdf Attached File- Added open_SyvI97zm.txt, #37760

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code