classification
Title: _decimal.Decimal constructed from tuple
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: georg.brandl Nosy List: georg.brandl, hac.man, mark.dickinson, python-dev, rhettinger, skrah
Priority: normal Keywords: patch

Created on 2012-09-07 22:47 by hac.man, last changed 2012-09-24 05:46 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
_decimal.diff hac.man, 2012-09-07 22:47 Patch for _decimal C code and additional unit test coverage. review
issue15882.diff skrah, 2012-09-08 13:14 review
Messages (16)
msg170017 - (view) Author: Aaron (hac.man) Date: 2012-09-07 22:47
I think I may have found a problem with the code that constructs Infinity from tuples in the C _decimal module.

# pure python (3.x or 2.x)
>>> decimal.Decimal( (0, (0, ), 'F'))
Decimal('Infinity')

# _decimal
>>> decimal.Decimal( (0, (0, ), 'F'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
decimal.InvalidOperation: [<class 'decimal.ConversionSyntax'>]

Also, there is no unit test coverage for constructing these special values from tuples either.  I have provided some that pass with the existing pure python code and with the modifications to the _decimal C code.

The unit tests can be applied to Python 2.7.x as well, if desired.  They would go in the ExplicitConstructionTest.test_explicit_from_tuples() method.
msg170035 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-08 06:50
Infinities do not have payloads. It's a bug in decimal.py that it
accepts non-empty coefficients when constructing infinities.

Since there was a corresponding unit test for as_tuple(), I've
kept the wrong representation for _decimal:

    # XXX non-compliant infinity payload.
    d = Decimal("Infinity")
    self.assertEqual(d.as_tuple(), (0, (0,), 'F') )


But this unit test is only executed for the Python version:

    # XXX coefficient in infinity should raise an error
    if self.decimal == P:
        d = Decimal( (0, (4, 5, 3, 4), 'F') )
        self.assertEqual(d.as_tuple(), (0, (0,), 'F'))
        d = Decimal( (1, (0, 2, 7, 1), 'F') )
        self.assertEqual(d.as_tuple(), (1, (0,), 'F'))


My suggestion is to disallow non-empty tuples for decimal.py
and change the infinity tuple to (0, (), 'F').
msg170036 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-08 07:13
> It's a bug in decimal.py that it
> accepts non-empty coefficients when constructing infinities.

On what grounds is it a bug?  I might call it a poor design decision, but it's clearly intentional.  There's even a unit test for it.
msg170037 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-08 07:36
Mark Dickinson <report@bugs.python.org> wrote:
> On what grounds is it a bug?  I might call it a poor design decision,
> but it's clearly intentional. There's even a unit test for it.

Maybe "bug" is not the right word. I'd call it a deviation from the
specification then. Do you happen to know what the intention was?

If from_tuple(..., 'F') accepts coefficients, logically Decimal("Infinity0"),
Decimal("Infinity123") etc. should be accepted as well. But they're not part
of the grammar and decimal.py rejects them.

http://speleotrove.com/decimal/daconvs.html
msg170038 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-08 07:57
> I'd call it a deviation from the specification then.
> Do you happen to know what the intention was?

Well, we're kinda outside the specification here.

Rightly or wrongly, Decimal('infinity').as_tuple() was originally implemented to return (0, (0,), 'F');  I'm fairly sure that was intentional, and I think that the Decimal constructor should continue to accept that tuple as a representation of infinity---else we're risking breaking existing code.

I think I may have muddied the waters at some later stage by modifying the constructor to ignore the second argument on input, so that (0, (), 'F') and (0, (0,), 'F') both work on input.  Or maybe it was already the case that the second argument was ignored;  I'm not sure about that.

I do agree that (0, (), 'F') is a better representation of infinity, and it would have been a little better if that had been used from the start, but I don't think it's worth the potential breakage or the deprecation cycles involved in 'fixing' this.


If it helps at all, it would probably be safe to disallow tuples other than () and (0,);  I doubt anyone's intentionally passing anything else here.

So:

+1 on allowing ((0, (0,), 'F')) as input in _decimal.  I do see this report as a regression from 3.2 that should be fixed.

-0 on changing Infinity.as_tuple() to (0, (), 'F'), and perhaps then deprecating ((0, (0,), 'F') as a legal input.
msg170043 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-08 10:44
It's an implementation detail of decimal.py to represent infinity with
a coefficient of '0' instead of the empty string:

>>> Decimal("inf")._int
'0'


So IMO the tuple representation exposes that implementation detail. I'm
still not sure why that was done or what the benefit was.

For cdecimal, initializing a coefficient for infinity is completely
meaningless, since infinities simply don't have a coefficient.

So the representation DecimalTuple(sign=0, digits=(0,), exponent='F'),
which I took over for compatibility, is factually wrong for cdecimal.

Of course, exponent='F' is also wrong. I don't mind that as much, because
there is a direct mapping to the input strings of the grammar.


I think the tuple representation isn't that useful to begin with. In all
use cases I've seen, people are a) reading the tuple an b) converting the
coefficient tuple back to a Python integer.


So, I'd really like to deprecate the whole interface in favor of either a
read-only interface:

    x.sign   -> 0 or 1 (perhaps even 1 or -1)
    x.coeff  -> Python integer
    x.exp    -> Python integer

If x is special, raise InvalidOperation. People usually need to check for
specials anyhow.

Or a tuple interface with an integer coefficient

    (sign=0, coeff=12345, exponent=99),

where special numbers are not allowed in accordance with http://speleotrove.com/decimal/damodel.html:

"Finite numbers are defined by three integer parameters"

"When a number has one of these special values, its coefficient and exponent are undefined."



That said, I don't mind if construction continues to work with the
implementation specific value of (0,), so let's do that.
msg170044 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-08 10:51
> So, I'd really like to deprecate the whole interface in favor of either a
> read-only interface:
>
>     x.sign   -> 0 or 1 (perhaps even 1 or -1)
>     x.coeff  -> Python integer
>     x.exp    -> Python integer
> 
> If x is special, raise InvalidOperation. People usually need to check for
> specials anyhow.

Sure;  that works for me.  I don't like the current interface any more than you do, and I've always found it unnecessarily awkward to turn finite decimals into triples and back again.

But I think that's a different issue.  What we have here is a real regression that seems likely to break existing code, and I think that should be fixed.
msg170045 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-08 10:54
> read-only interface:
>
>     x.sign   -> 0 or 1 (perhaps even 1 or -1)
>     x.coeff  -> Python integer
>     x.exp    -> Python integer

The only slightly awkward thing with this (for high-precision decimals) is the quadratic conversion (well, okay, perhaps subquadratic depending on what algorithms are being used, but still ...) required to get from the decimal coefficient to a Python integer, and vice versa.  It may be better to use a (byte)string of digits instead of a Python integer for the coefficient.
msg170046 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-08 11:00
Mark Dickinson <report@bugs.python.org> wrote:
> > So, I'd really like to deprecate the whole interface in favor of either a
> > read-only interface:
> 
> But I think that's a different issue.

Yep. I was using all this as a rationale that the current tuple interface
does not matter all that much.
msg170049 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-08 13:14
Aaron, did you encounter this problem in a real world application?
I'm asking because it would be interesting to know if people use this.
Google is pretty silent on "Decimal((0, (0,), 'F'))", but perhaps
that is not an ideal search term.


Mark, I'm uploading a minimal patch. I'm OK with that going into 3.3.0,
if you manage to convince Georg. :)
msg170062 - (view) Author: Aaron (hac.man) Date: 2012-09-08 19:26
I did not encounter this in a regular application.  I do use the decimal module, and was excited to see the adoption of a faster C version, so I was just reading through the code to see how it worked.  I can't think of a situation where I would need to construct a decimal from a tuple and not a string or some other numeric type, though.  For what it's worth, I think that as long as construction from tuples is supported, Decimal(d.as_tuple()) should work for all Decimal objects d.
msg170207 - (view) Author: Roundup Robot (python-dev) Date: 2012-09-10 17:42
New changeset 97e53273f423 by Stefan Krah in branch 'default':
Issue #15882: Change _decimal to accept any coefficient tuple when
http://hg.python.org/cpython/rev/97e53273f423
msg170266 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-11 06:55
Thanks for the report and the patch. I used another approach that still
validates the digits in the coefficient tuple even if it is not used.

decimal.py allows any coefficient:

>>> Decimal((0, ('n', 'a', 'n'), 'F'))
Decimal('Infinity')

_decimal raises:

>>> Decimal((0, ('n', 'a', 'n'), 'F'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: coefficient must be a tuple of digits

I'm leaving the issue open: If some release blocker arises, we could get this
into 3.3.0-rc3.
msg170884 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-09-21 13:26
Georg, *if* there is an rc3, could you add 97e53273f423 to it?

The commit is a backwards compatibility fix for constructing
infinities from tuples with arbitrary coefficients.


If there's no rc3, the issue can just be closed as fixed.
msg170957 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-09-22 06:53
Done.
msg171101 - (view) Author: Roundup Robot (python-dev) Date: 2012-09-24 05:46
New changeset d22ea2aa1057 by Stefan Krah in branch 'default':
Issue #15882: Change _decimal to accept any coefficient tuple when
http://hg.python.org/cpython/rev/d22ea2aa1057
History
Date User Action Args
2012-09-24 05:46:51python-devsetmessages: + msg171101
2012-09-22 06:53:21georg.brandlsetstatus: open -> closed

messages: + msg170957
2012-09-21 13:26:17skrahsetassignee: skrah -> georg.brandl

messages: + msg170884
nosy: + georg.brandl
2012-09-11 06:56:50skrahsetresolution: fixed
stage: resolved
2012-09-11 06:55:48skrahsetmessages: + msg170266
2012-09-10 17:42:05python-devsetnosy: + python-dev
messages: + msg170207
2012-09-08 19:26:47hac.mansetmessages: + msg170062
2012-09-08 13:14:37skrahsetfiles: + issue15882.diff

messages: + msg170049
2012-09-08 11:00:37skrahsetmessages: + msg170046
2012-09-08 10:54:04mark.dickinsonsetmessages: + msg170045
2012-09-08 10:51:09mark.dickinsonsetmessages: + msg170044
2012-09-08 10:44:23skrahsetassignee: skrah
type: behavior
messages: + msg170043
2012-09-08 10:14:17mark.dickinsonsetnosy: + rhettinger
2012-09-08 07:57:05mark.dickinsonsetmessages: + msg170038
2012-09-08 07:36:38skrahsetmessages: + msg170037
2012-09-08 07:13:27mark.dickinsonsetmessages: + msg170036
2012-09-08 06:50:46skrahsetnosy: + mark.dickinson
messages: + msg170035
2012-09-07 23:01:05hayposetnosy: + skrah
2012-09-07 22:47:55hac.mancreate