python / cpython

The Python programming language
https://www.python.org
Other
62.43k stars 29.97k forks source link

Decimal class error messages for integer division aren't good #65426

Closed fa5ec958-6532-42d7-99e4-e0654bdcb9f7 closed 9 years ago

fa5ec958-6532-42d7-99e4-e0654bdcb9f7 commented 10 years ago
BPO 21227
Nosy @tim-one, @rhettinger, @facundobatista, @mdickinson, @skrah, @leewz
Dependencies
  • bpo-19232: Speed up _decimal import
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/skrah' closed_at = created_at = labels = ['type-feature', 'library', 'docs'] title = "Decimal class error messages for integer division aren't good" updated_at = user = 'https://github.com/leewz' ``` bugs.python.org fields: ```python activity = actor = 'skrah' assignee = 'skrah' closed = True closed_date = closer = 'skrah' components = ['Documentation', 'Library (Lib)'] creation = creator = 'leewz' dependencies = ['19232'] files = [] hgrepos = [] issue_num = 21227 keywords = [] message_count = 13.0 messages = ['216253', '216492', '216496', '216510', '216521', '216531', '216565', '216573', '216582', '216589', '216615', '216622', '229302'] nosy_count = 7.0 nosy_names = ['tim.peters', 'rhettinger', 'facundobatista', 'mark.dickinson', 'skrah', 'docs@python', 'leewz'] pr_nums = [] priority = 'low' resolution = 'wont fix' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue21227' versions = ['Python 3.5'] ```

    fa5ec958-6532-42d7-99e4-e0654bdcb9f7 commented 10 years ago

    Python's decimal.Decimal doesn't seem to like taking modulo or intdiv of large Decimals by integers (where "large" depends on how many digits are internally stored).

        >>> from decimal import *
        >>> getcontext().prec
        28
        >>> Decimal(10**29)%1
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        decimal.InvalidOperation: [<class 'decimal.DivisionImpossible'>]
        >>> getcontext().prec=50
        >>> Decimal(10**29)%1
        Decimal('0')

    Same for Decimal(10**29)//1

    This is a logical problem: "Alice has a 100-digit number which begins with 1543256. What is the last digit?"

    But I found the error hard to understand. Searching for "DivisionImpossible" didn't turn up anything immediate (it wasn't added to the official docs?). I was most of the way through writing out a question to StackOverflow when it clicked. (Also, why is it an InvalidOperation that holds an exception as a message? Never saw that before.)

    Suggestions:

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    It is hard to get fine grained error messages in _decimal, since the errors come from libmpdec. A clean solution would require changes to libmpdec, and I'm reluctant to do that right now.

    It is certainly possible to document DivisionImpossible etc.

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    Meanwhile, the pure Python decimal versions prior to Python 3.2 have better error messages.

    Right now in Python 3.3+ it is hard to import the Python version without going into contortions, but that may be fixed in bpo-19232.

    fa5ec958-6532-42d7-99e4-e0654bdcb9f7 commented 10 years ago

    Fine grained? Do you mean that the error can't be distinguished from other such errors? Or that it's difficult to attach the message to DivisionError? I thought DivisionError was always about precision.

    I looked up the error in libmpdec: "This occurs and signals invalid-operation if the integer result of a divide-integer or remainder operation had too many digits (would be longer than precision). The result is [0,qNaN]." (http://speleotrove.com/decimal/daexcep.html)

    Now I'm more confused. Though it mentions precision, it is talking about the *result's precision being too large (something which shouldn't happen with Python's unbounded ints, right?), rather than being unable to give a sane answer due to not having *enough digits. That's also what the 2.7 error is: decimal.InvalidOperation: quotient too large in //, % or divmod

    I'm very much content with documenting it, but if possible, I'd like to understand whether this is an issue to take up with libmpdec.

    P.S.: As a side-note to others, Python floats allows float%int even when precision isn't high enough, and seems to always returns 0.0 with no warning. So behavior is inconsistent, if that's important to anyone here.

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    My apologies if that wasn't clear: "fine grained" refers to the exception messages. A function can raise InvalidOperation for different reasons.

    decimal.py gives a specific error message in each case. libmpdec just signals the standard conforming InvalidOperation.

    The relevant passage from the standard is here:

    http://speleotrove.com/decimal/daops.html#refremain

    "This operation will fail under the same conditions as integer division."

    decimal.py, decNumber and libmpdec all do the same thing, so there is no libmpdec issue other than that the error *messages* could be improved.

    I fully understand if you find the behavior surprising (after all the remainder fits in the precision), but as long as we follow the standard we can't change that.

    fa5ec958-6532-42d7-99e4-e0654bdcb9f7 commented 10 years ago

    Nah. I found it surprising at first, but like I said, it's like the computer is given the first 28 digits of a number and then asked to figure out the 30th digit.

    What I'm confused about is how it fits the definition of "division impossible" given by libmpdec's docs (about the result size), and whether you're saying it's difficult to translate the [<class 'decimal.DivisionImpossible'>] part to an error message.

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    In the case of DivisionImpossible it would actually be possible to use the error message 'quotient too large in //, % or divmod'.

    But that's just one condition. In the case of InvalidOperation there are something like 30 different error messages.

    rhettinger commented 10 years ago

    It is certainly possible to document DivisionImpossible etc.

    This should not be done. We've made strong efforts to not extend the Decimal Arithmetic Specification.

    Also, the API already suffers from high complexity. Adding more exceptions to the mix just makes the module harder to learn.

    Instead, you are welcome to add more specific text messages to the existing (and standards compliant) exceptions:

    raise InvalidOperation("Not enough precision to compute the answer.")

    I found it surprising at first, but like I said, it's like the computer is given the first 28 digits of a number and then asked to figure out the 30th digit.

    People are always surprised when their mental model conflicts with the actual design model. That doesn't mean the implementation should change.

    There may be a documentation issue, but then again, people who are "surprised" usually haven't studied the docs in detail.

    My overall sense for this bug report is that there isn't a real problem that needs to be solved. AFAICT, this particular "confusion" has never arisen before (i.e. bug reports, questions in python classes, posts on stackoverlow, blog posts, etc). Likewise, the issue does not seem to have ever arisen in the many other non-Python implementations of the decimal spec.

    I recommend that this either be closed or be limited to tweaking some of the error messages for existing exceptions.

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    Raymond Hettinger \report@bugs.python.org\ wrote:

    > It is certainly possible to document DivisionImpossible etc.

    This should not be done. We've made strong efforts to not extend the Decimal Arithmetic Specification.

    The exceptions are already there:

    Python 2.7.3 (default, Mar 13 2014, 11:03:55)
    [GCC 4.7.2] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import decimal
    >>> print(decimal.DivisionImpossible.__doc__)
    Cannot perform the division adequately.
    This occurs and signals invalid-operation if the integer result of a
    divide-integer or remainder operation had too many digits (would be
    longer than precision).  The result is [0,qNaN].

    The specification distinguishes between "conditions" and "signals". It is true that the conditions are not technically raised, but they are very much "subclasses" of InvalidOperation.

    Cowlishaw himself makes the distinction between InvalidOperation and IEEEInvalidOperation. The latter groups all conditions together:

    #define DEC_IEEE_754_Invalid_operation (DEC_Conversion_syntax |     \
                                            DEC_Division_impossible |   \
                                            DEC_Division_undefined |    \
                                            DEC_Insufficient_storage |  \
                                            DEC_Invalid_context |       \
                                            DEC_Invalid_operation)

    So while I don't particularly care whether we document the conditions or not, I don't think doing so would extend the specification.

    fa5ec958-6532-42d7-99e4-e0654bdcb9f7 commented 10 years ago

    Total list of issues now:

    I checked all of these just now on my 3.2.3 (Windows). These issues are a CHANGE from 3.2 to 3.3. For example:

    I assume that the issues also apply to the other InvalidOperation types. So I guess what I'm really asking for is to pull forward the 3.2 docs and messages.

    That doesn't mean the implementation should change.

    Er, I was explaining why it wasn't really surprising once I thought about it.

    There may be a documentation issue, but then again, people who are "surprised" usually haven't studied the docs in detail.

    To clarify:

    I'm not asking for a change in behavior. I did point out that it was inconsistent with regular floats, in case consistency with Pyfloats was a thing that mattered. But I don't care about that. I only worry about anyone else who would use Decimal coming across the same unhelpful error.

    AFAICT, this particular "confusion" has never arisen before (i.e. bug reports, questions in python classes, posts on stackoverlow, blog posts, etc). Likewise, the issue does not seem to have ever arisen in the many other non-Python implementations of the decimal spec.

    I agree it'd be (very) rare, but part of the reason why it might not appear online is that the issues at the top of this email are relatively new (3.3).

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    leewz report@bugs.python.org wrote:

    • Error message for DivisionImpossible is [<class 'decimal.DivisionImpossible'>] instead of an actual error message.

    No, the error message for the *signal that is raised (InvalidOperation) lists the *condition that triggered the signal (DivisionImpossible).

    I followed the recommendation at:

    http://speleotrove.com/decimal/daexcep.html#refexcep

    "It is recommended that implementations distinguish the different conditions listed above, and also provide additional information about exceptional conditions where possible (for example, the operation being attempted and the values of the operand or operands involved)."

    Distinguishing the conditions is easy, adding additional information in all cases would require changes to libmpdec.

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 10 years ago

    The idea behind the list as the exception message is this: If multiple conditions would have raised the signal they are all listed, while the "highest ranking" signal is the one that is ultimately raised.

    >>> from decimal import *
    >>> c = getcontext()
    >>> for v in c.traps:
    ...     c.traps[v] = True
    ... 
    >>> 
    >>> Decimal(8) ** 1000000000000000
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    decimal.Overflow: [<class 'decimal.Overflow'>, <class 'decimal.Inexact'>, <class 'decimal.Rounded'>]

    Exception precedence is listed here at the bottom of the page:

    http://speleotrove.com/decimal/daexcep.html

    5531d0d8-2a9c-46ba-8b8b-ef76132a492c commented 9 years ago

    I guess there's not much to be done here.