Arraymancer icon indicating copy to clipboard operation
Arraymancer copied to clipboard

Create more descriptive error types

Open mratsim opened this issue 6 years ago • 4 comments

Instead of using ValueError and IndexError everywhere it is probably more informative to introduce more descriptive error types.

For example:

  • IncompatibleShapeError (for tensors that can't be multiplied together)
  • UnreachableError (for when/if elif construct that should cover all cases)
  • NotImplementedError (for missing features or for proc/method that must be overloaded, e.g. forward and backward)

mratsim avatar Sep 09 '17 19:09 mratsim

@mratsim This looks like something I can do. Is this still a valid issue ?

D-K-E avatar Oct 08 '17 02:10 D-K-E

Yes it's still a valid issue. Here is the target design.

  • All exceptions are currently returned in inline procs that begins with "check_".
  • All calls must be gated behind when compileOption("boundChecks")
    • Besides I would like to change calls to use 2 lines for readability.
       when compileOption("boundChecks"):
           check_very_long_foo_name(a,b)
      
  • "assert" should be replaced by an appropriate exception
  • Exceptions should give as much context as possible (input shapes if relevant) so that debugging experience is easier.
  • Testing exceptions should be disabled in release mode: https://github.com/mratsim/Arraymancer/blob/78a92a4b3b3d8c931ed82aa385a4a8692e3472c0/tests/tensors/test_accessors.nim#L34-L47

This is mostly done, there might be a few stragglers left.

Now I would like exceptions more descriptive than ValueError and IndexError where relevant (Indexing a tensor or shape [2, 3] with [10, 10] should still be an IndexError).

The exceptions should inherit from Nim core exceptions I think so that generic Nim tools can still work. For example supposing we introduce IncompatibleShapeError and SliceError, the first inherits from ValueError, the second from IndexError. I'll let you propose your hierarchy.

Also the number of exceptions is growing so it might be easier for you to move them all in a single file. That's fine.

Thanks for tackling this !

mratsim avatar Oct 08 '17 07:10 mratsim

I started a separate file as you've suggested. I am also storing the hierarchy of the exceptions in the same file. For now, I am storing it inside utils folder. Just a quick question. Should I include the exceptions file in all the files where custom exceptions occur, or does an initial import in the arraymancer.nim takes care of that ?

D-K-E avatar Oct 08 '17 22:10 D-K-E

Don't forget to rebase against master. I removed all includes and use imports now. Most of the checks if not all the checks are in a ./private/p_checks file.

If you find checks that are not there, move them and this way you only have to import your exceptions in the p_checks file.

mratsim avatar Oct 09 '17 11:10 mratsim