bugGNU Octave - Bugs: bug #45957, Error "A(I) = X: X must have...

 
 

bug #45957: Error "A(I) = X: X must have the same size as I" not very informative

Submitter:  Lachlan Andrew <lachlan>
Submitted:  Mon 14 Sep 2015 10:48:46 AM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Lachlan Open/Closed:  * Closed
Release:  * 4.0.0 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 10 Oct 2015 02:33:16 AM UTC, comment #9: 

Changing report to "In Progress", although It may be safe to just close this since the patch has been accepted.

Rik <rik5>
Group administrator
Fri 02 Oct 2015 07:51:56 PM UTC, comment #8: 

I pushed the following changeset:

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

There are still some things I'd like to clean up but I thought it best to get this change committed sooner rather than later sine it touches so many files and I'm about to start touching many files (anything that uses error_state and probably more).

John W. Eaton <jwe>
Group administrator
Fri 02 Oct 2015 12:16:34 AM UTC, comment #7: 

I'm attaching and edited patch that applies on top of the following changeset:

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


(file #35030)

John W. Eaton <jwe>
Group administrator
Thu 01 Oct 2015 05:30:22 PM UTC, comment #6: 

I think this change is OK except for some style issues (some things I noticed: don't use tabs, write foo () for function calls, not foo(), avoid using sprintf to write into a fixed-size buffer).  I'd also like to start catching exceptions using const reference unless the exception object will be modified.  It also needs a commit message in the ChangeLog style we usually use.

I have some pending changes that will also touch a lot of files, so maybe it would be best for me to commit your change and fix some of these style issues, then commit my change?

John W. Eaton <jwe>
Group administrator
Thu 24 Sep 2015 06:21:13 AM UTC, comment #5: 

Thanks again for suggesting using exceptions, John.  They make things much more tractable!

I've added a patch that returns messages like

abc(_,3,_); but abc has size 2x2x2
xyz(0+1i); subscripts must be real (forgot to set i or j?)
foo(_,<cell>); subscripts must be integers 1...(2^31)-1 or logical


It derives an exception class  idx-except  from  octave-execution-error (and so should be caught by the outermost failsafe in the main loop).  Particular error types are derived from idx-except to customise the error message.

This object gets thrown when the invalid index is detected, caught and rethrown when the index position is known, and finally caught when the variable name is known.

The patch touches much more code than I had expected, but I think that is necessary.

Please let me know if more work is required on it.

Thanks,
Lachlan

(file #34971)

Lachlan Andrew <lachlan>
Thu 17 Sep 2015 02:05:04 AM UTC, comment #4: 

They're both good suggestions, John.

I made a start on specifying which dimension caused the problem, but the issue with invalid indices is flagged several levels deeper than out-of-range indices.  I got lost in the tangle of passing location references through subroutine calls, and so decided to submit the cleaner patch first.

The idea of exceptions is great.  One issue is that the piece of code that knows which index is in doesn't know which variable it is -- because it could be (A+B)(1,-1).  The parenthesis-handling code would probably have to partially format the error message, and then re-throw to the variable-handling code.  Who would then catch the exception for (A+B)(1,-1)?  I suppose that could go way up in the code that evaluates a generic expression.

Ideally, I'd like the error to be something like
error:  Accessing varname(.,index).
for example


error: accessing A(-1)
error: accessing B(1e50,.)
error: accessing coord(.,1.3,.,.)


For out-of-range errors, I'd like

error: accessing varname(x,y,z) but varname has size AxBxC.

If we get

  x = [1 2 3]
  x(2,1)
==>
  error: index to dimension 1 is out of bounds; value 2 out of bound 1

we don't know if  x  is a singleton, or we've just swapped the row and column indices.  (I deal a lot with lists rather than true vectors, so I get sloppy.)

The cast to double is problematic for indices greater than 2^52, which would only occur in the most extreme sparse matrices.  Are there other problems?  I was just trying to avoid having two nearly-identical sections of code to maintain.  I suppose we could convert the index to a string and have the octave_idx_type and double calls both pass a string to a common function.


A question about etiquette: I'm preparing a patch to make distribution functions (like unifcdf) give more explicit errors.  Should I add that to this bug, or open a new one?

Thanks!

Lachlan Andrew <lachlan>
Wed 16 Sep 2015 12:43:01 PM UTC, comment #3: 

Instead of using a global context variable that must always be set to get better information in the error message, isn't this a place where throwing an exception would also solve the problem, and then only incur the overhead when the exception is found?  That way, indexing code that wants to catch the exception and also add info about the context could do so, and otherwise, you would just get a more generic error about a failed indexing operation.

I realize that using an exception would be a pretty big departure from existing practice for error handling in Octave, but I think it should be considered.

John W. Eaton <jwe>
Group administrator
Wed 16 Sep 2015 12:40:01 PM UTC, comment #2: 

I agree that more information would be better.

It might also be good to display the position of the index if possible.  For example, something like this:


  x = ones (30, 20, 10);
  x(12,12,12)

==>
  error: A(I,J,...): index to dimension 3 out of bounds; value 12 out of bound 10


We already have a function, gripe_index_out_of_range, that does this.  Maybe it just needs to be used in more places?

Also, I don't think that casting the index value to double is the right thing to do.

John W. Eaton <jwe>
Group administrator
Mon 14 Sep 2015 11:56:32 PM UTC, comment #1: 

This is a really nice idea for an enhancement! Just a couple of other messages to consider refining:

>> Q=zeros(1e7,1e7);

error: out of memory or dimension too large for Octave's index type

>> Q=zeros(1e20,1);

error: out of memory or dimension too large for Octave's index type

I wonder if the message could tell you explicitly if your numel>2^31 (or 63) and whether there is insufficient memory.

The sparse function does gives this in some cases:

>> s = sparse(1,1e20,0)

error: sparse: subscript indices must be either positive integers less than 2^31 or logicals

but not others:

>> s = sparse(1,1e20)

error: out of memory or dimension too large for Octave's index type





Ceral Paquet <octavebugs>
Mon 14 Sep 2015 10:48:46 AM UTC, original submission:  

The following errors omit much of the information known to Octave that would be useful to the programmer:
   "A(I) = X: X must have the same size as I"
   "A(I,J,...) = X: dimensions mismatch"
   "subscript indices must be either positive integers less than 2^63 or logicals"

The attached patch produces more informative messages.  For the first two, it specifies the actual dimensions of the LHS and RHS.  For the third, it specifies the offending index.

For the third, it would be much more useful to say which variable is being accessed.  That would help in code like

  A(B(C(j), D(k)), E(F(m), G(n)))

where many variables are accessed in the same line of code.  It would also be of some use to specify which dimension is in error. 

However, it seems that information would have to be passed through many layers of function calls, which would have a (slight?) performance hit.  Is it worth passing that info, or perhaps providing a compile-time option?  In the latter case, there wouldn't need to be too many #defines; it could produce a CONTEXT macro that gets passed to the functions, which is either empty or a pointer to the desired context.

Any suggestions?

Also, I realise that this patch affects things right at the heart of Octave, and so the maintainers will be reluctant to apply it.  Is there something I can do to improve its chances?

Lachlan Andrew <lachlan>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35030:  idx-errors-patch.txt added by jwe (114KiB - text/plain)
file #34971:  idx_errors_ignore_indent.patch added by lachlan (79KiB - text/x-patch - idx_errors, ignoring whitespace for easier review)
file #34970:  idx_errors.patch added by lachlan (112KiB - text/x-patch - Report variable name and index position when reporting index errors)
file #34874:  index_errors.patch added by lachlan (11KiB - text/x-patch)

 

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 jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by octavebugs (Posted a comment)
  • -email is unavailable- added by lachlan (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-10-13 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2015-10-10 rik5 StatusPatch Submitted In Progress
    2015-10-02 jwe Attached File- Added idx-errors-patch.txt, #35030
    2015-09-24 lachlan Attached File- Added idx_errors_ignore_indent.patch, #34971
    2015-09-24 lachlan Attached File- Added idx_errors.patch, #34970
    2015-09-15 mtmiller StatusNone Patch Submitted
    2015-09-14 lachlan Attached File- Added index_errors.patch, #34874

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code