Unify error for trying to invert non-invertible elements

[Martin R]

Dear all,

currently, when trying to invert a non-invertible element, one of the following errors is raised (found using `grep -r --include=*.{py,pyx} --color -nH --null -e "Error(.*invertible" *`):

• 21 ValueError('{} is not invertible')
• 11 ZeroDivisionError("element is not invertible")
• 10 TypeError('the A-basis is defined only when 2 is invertible')
• 8 ArithmeticError("self must be invertible to have a multiplicative order")
• 2 RuntimeError("morphism is not invertible")
• 2 NotImplementedError("matrix must be invertible")

Travis and I would like to propose to unify these to "ArithmeticError".  If nobody objects, I would prepare a PR within the next few days.

Best wishes,

Martin

[Dave Morris]

-1 from me.

Looking at just a few uncovered some that I think are definitely not ArithmeticError.

Examples:
There is an occurrence of ValueError('{} is not invertible') in the lift_isometry method of cliffordalgebra.py. Lifting an isometry is clearly not an arithmetic operation. ValueError is correct here.
There is an occurrence of TypeError('the A-basis is defined only when 2 is invertible') in the method to find an `A`-basis of an Iwahori-Hecke algebra. Finding such a basis is clearly not an arithmetic operation. Perhaps this should be a ValueError instead of a TypeError, though.
RuntimeError("morphism is not invertible"). I think that finding the inverse of a morphism between two objects of a category is clearly not an arithmetic operation. Perhaps this should be a ValueError rather than a RuntimeError.

If you want to unify, I think ValueError might work. But I think they all need to be inspected, not just assume a single error type fits.

> --
> You received this message because you are subscribed to the Google Groups "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/d0637584-4908-4101-8e1b-74ec0477ff84n%40googlegroups.com.

[David Roe]

I agree that there are cases that shouldn't be changed to an ArithmeticError, and that grepping for "invertible" isn't sufficient.  But I think with a narrower scope this change is a good idea: if the error arises from attempting to invert a non-invertible element of a ring.

So +1 from me, with a manual check that the change seems appropriate.

David

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/4A0E1F77-27A7-4DF3-BB18-636853BF4CC0%40deductivepress.ca.

[Travis Scrimshaw]

Dave, I agree that it is quite possible that not all of them are correct non-invertible elements in a ring. Of course, we would go through an examine them more individually as David suggested.

I was originally thinking ValueError as well, but, as Martin pointed out, ArithmeticError is a bit more specific. So that gets my vote.

Best,

Travis

[Kwankyu Lee]

+1 from me on this specific case.

On the other hand, there are similar arbitrariness in other cases. So I think there should be a broader discussion on what error should be raised in what situation in mathematical context. This is what I think:

A method (or function) takes objects as input and computes an output.  The INPUT block defines coarsely  the intended class of mathematical objects. 

TypeError: the type (that can be checked by isinstance(obj, class)) of the input object does not belong to the intended class of mathematical objects

ValueError: the particular input object is not suitable as input

ArithmeticError: the particular input object is not suitable for arithmetic (sum, product, quotient, and the like) operation 

ZeroDivisionError: the method performs division but the input is zero

NotImplementedError: there is no problem with the input object but the method is incapable to compute appropriate output.

RuntimeError: The method somehow cannot perform the computation. Perhaps a catchall error.

[Kwankyu Lee]

A method (or function) takes objects as input and computes an output.  The INPUT block defines coarsely  the intended class of mathematical objects. 

TypeError: the type (that can be checked by isinstance(obj, class)) of the input object does not belong to the intended class of mathematical objects

ValueError: the particular input object is not suitable as input

ArithmeticError: the particular input object is not suitable for arithmetic (sum, product, quotient, and the like) operation 

ZeroDivisionError: the method performs division but the input is zero

NotImplementedError: there is no problem with the input object but the method is incapable to compute appropriate output.

RuntimeError: The method somehow cannot perform the computation. Perhaps a catchall error.

I may add the above to the developer guide. Any comments? 

[Travis Scrimshaw]

I would be vague about a TypeError versus a ValueError. These are used in various ways by different authors over different periods. It can also be very hard to make this rigorous. For example, for something accepting integer inputs, then 2/2 fails the isinstance() check but shouldn't throw an error. Likewise 1/2 is a bad value that cannot be tested by the isinstance() check alone. Actually, I might consider putting them together and letting the programmer decide which is most appropriate (of course, there are clear cases, such as a list compared to a number).

The ArithmeticError and ZeroDivisionError are fine with me.

I think NotImplementedError is obvious and should be stated in such a way: "the input is for a feature not yet implemented"

For RuntimeError, I would make it sound like it tells you there is serious error occurring as it doesn't fall into any other error categories. This actually makes it the opposite of a catchall error.

Best,

Travis

[David Roe]

On Thu, Feb 8, 2024 at 8:18 PM 'Travis Scrimshaw' via sage-devel <sage-...@googlegroups.com> wrote:

For RuntimeError, I would make it sound like it tells you there is serious error occurring as it doesn't fall into any other error categories. This actually makes it the opposite of a catchall error.

The main scenario when I use a RuntimeError is when an assumption that I never expected to be violated fails.  I agree with Travis that catchall is not the right description for this error type.

David

[dmo...@deductivepress.ca]

Description of RunTimeError from docs.python.org: "Raised when an error is detected that doesn’t fall in any of the other categories. The associated value is a string indicating what precisely went wrong."

It is for exceptions that cannot be categorized, so I believe it is indeed just a catch-all error (but must have a description), and there is no expectation that such an error is particularly serious. (A situation where "an assumption that I never expected to be violated fails" can raise AssertionError.)

[Travis Scrimshaw]

You're misunderstanding what a catch-all means. It means *any* type (of error) is reasonable. To put it mathematically, catch-all means union (of sets), but the Python doc means difference (of sets).

An assertion is slightly different. It can (in principle) be turned off and is just used for internal checking. While there is large overlap, it is not quite the same (e.g., if code is assuming a certain conjecture, it should not use assert to check validity of assumptions).

Best,

Travis

[Nils Bruin]

The main relevance of classifying errors properly in python is for try/except. Generally, a try/except should be tight both in code guarded and in errors caught. An `except RuntimeError` is almost certainly a bad idea. `except ZeroDivisionError` looks quite reasonable. `except ValueError` or `except ArithmeticError` can already easily catch a little too much. `except TypeError` looks very suspicious when what is intended to be caught is an object (of the right type) to which a (partial) "inversion" map cannot be applied. "NotImplemented" should probably not be caught either: implementing the missing functionality is ideally the way to avoid that error from occurring.

Error objects do have more info than just their type and an except clause can investigate this and decide to reraise. That's a huge pain to program (and potentially slow), so in practice that's almost never done. The type of an error is in practice the only granularity for try/except.

Reclassifying errors should probably happen with an eye towards the utility for try/except. That means that reclassifying existing error will break existing code, so you better have a very good reason. For "RuntimeError" I agree, because there the developer (unintentionally?) signals "this shouldn't be caught". So you'll have to go in, analyse the intent and reassess if that error should indeed not be caught.

For the other ones it depends: if it's an object that can end up in polymorphic code then aligning errors makes sense, but otherwise the pain of breakage probably means the change is not worth it.

[Kwankyu Lee]

On Friday, February 9, 2024 at 10:18:45 AM UTC+9 Travis Scrimshaw wrote:

I would be vague about a TypeError versus a ValueError. These are used in various ways by different authors over different periods.

Helping an author in choosing the most appropriate one among sometimes confusing array of Exceptions is the aim of my proposal. It would be impossible to write a guide consistent with existing code in sage. It is forward-oriented. No plan to update sage code according to the guide.

 

It can also be very hard to make this rigorous.

TypeError and ValueError is the most confusing case. Python documentation does not help much. Giving a rough definition in sage context would be helpful.

For example, for something accepting integer inputs, then 2/2 fails the isinstance() check but shouldn't throw an error. Likewise 1/2 is a bad value that cannot be tested by the isinstance() check alone. Actually, I might consider putting them together and letting the programmer decide which is most appropriate (of course, there are clear cases, such as a list compared to a number).

 Sorry, I should not have used "isinstance()". I wanted to emphasize that TypeError should be used to reject input objects in certain "type" or "class" or "set"

For example, if a method accepts a polynomial as input. Then it may emit TypeError because the input is a polynomial over symbolic ring while it may emit ValueError because the specific input is not appropriate for the method to work. Of course, if the input is not a polynomial, this is just user's fault (no need to detect this in general)

Right, there is no perfect or rigorous definition. But we may give a certain guide or hint with which the author chooses an appropriate one between TypeError and ValueError.

In the order of specificity,

INPUT description > TypeError > ValueError > ArithmeticError > ZeroDivisionError 

If no other type of error is appropriate, then RuntimeError.

 

[Kwankyu Lee]

On Thursday, February 8, 2024 at 11:15:34 AM UTC+9 Kwankyu Lee wrote:

Implemented in 

https://github.com/sagemath/sage/pull/37311 

[Kwankyu Lee]

We are about to ship

https://github.com/sagemath/sage/pull/37311 

which adds a guide on choosing exceptions in sage context. Please come and review.