decimal.py: type conversion in context methods

[skrah]

BPO	7633
Nosy	@rhettinger, @facundobatista, @mdickinson, @skrah
Files	issue7633_jjconti.patch: Patch from Juanjo Conti for issue 7633 issue7633_jjconti2.patch: New Patch for issue 7633 issue7633_jjconti3.patch: 3rd patch issue7633_jjconti4.patch: 4th patch issue7633_jjconti4_metd.patch issue7633_jjconti5_metd.patch

^{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:

assignee = 'https://github.com/mdickinson'
closed_at = <Date 2010-02-18.14:53:06.918>
created_at = <Date 2010-01-04.13:01:12.082>
labels = ['easy', 'type-bug', 'library']
title = 'decimal.py: type conversion in context methods'
updated_at = <Date 2010-02-18.14:53:06.917>
user = 'https://github.com/skrah'

bugs.python.org fields:

activity = <Date 2010-02-18.14:53:06.917>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2010-02-18.14:53:06.918>
closer = 'mark.dickinson'
components = ['Library (Lib)']
creation = <Date 2010-01-04.13:01:12.082>
creator = 'skrah'
dependencies = []
files = ['15751', '15760', '16032', '16052', '16245', '16251']
hgrepos = []
issue_num = 7633
keywords = ['patch', 'easy']
message_count = 29.0
messages = ['97207', '97259', '97260', '97261', '97268', '97288', '97304', '97308', '97317', '98212', '98213', '98214', '98216', '98346', '98347', '98349', '98351', '98354', '98497', '98526', '98527', '98534', '98535', '98541', '98544', '99475', '99502', '99507', '99509']
nosy_count = 5.0
nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'jjconti', 'skrah']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue7633'
versions = ['Python 2.7', 'Python 3.2']

[skrah]

I think that context methods should convert arguments regardless of position:

Python 2.7a0 (trunk:76132M, Nov  6 2009, 15:20:35) 
[GCC 4.1.3 20080623 (prerelease) (Ubuntu 4.1.2-23ubuntu3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from decimal import *
>>> c = getcontext()
>>> c.add(8, Decimal(9))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/decimal.py", line 3866, in add
    return a.__add__(b, context=self)
TypeError: wrapper __add__ doesn't take keyword arguments
>>> 
>>> c.add(Decimal(9), 8)
Decimal('17')
>>> 

Also, perhaps c.add(9, 8) should be allowed, too.

[jjconti]

The same happens with other Context methods, like divide:

Python 2.6.2 (release26-maint, Apr 19 2009, 01:56:41) 
[GCC 4.3.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from decimal import *
>>> c = getcontext()
>>> c.divide
<bound method Context.divide of Context(prec=28, rounding=ROUND_HALF_EVEN, Emin=-999999999, Emax=999999999, capitals=1, flags=[], traps=[Overflow, InvalidOperation, DivisionByZero])>
>>> c.divide(2,3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/decimal.py", line 3966, in divide
    return a.__div__(b, context=self)
TypeError: wrapper __div__ doesn't take keyword arguments
>>> c.divide(Decimal(2),3)
Decimal('0.6666666666666666666666666667')
>>> c.divide(6,Decimal(2))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/decimal.py", line 3966, in divide
    return a.__div__(b, context=self)
TypeError: wrapper __div__ doesn't take keyword arguments

[mdickinson]

I agree that this would be desirable. And yes, c.add(9, 8) should be allowed, too.

Anyone interested in producing a patch?

[jjconti]

I've been reading http://speleotrove.com/decimal and seems not to be an explicit definition about this. I'm thinking in a patch I could supply tomorrow.
What about the idea of changing the implementation from:

    return a.__add__(b, context=self)

to

    return Decimal(a+b,context=self)

?

[mdickinson]

What about the idea of changing the implementation from:

   return a.\_\_add__(b, context=self)

to

    return Decimal(a+b,context=self)

?

I think it would be better to convert the arguments a and b to Decimal before doing the addition. In the case of addition, it doesn't make much difference: for integers a and b, Decimal(a+b) rounded to the current context should be the same as Decimal(a) + Decimal(b). But for e.g., division, Decimal(a/b) is potentially different from Decimal(a)/Decimal(b).

There's also the issue that the context methods can return 'NotImplemented'. For example:

Python 2.7a1+ (trunk:77217:77234, Jan  2 2010, 15:45:27) 
[GCC 4.2.1 (Apple Inc. build 5646) (dot 1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from decimal import *
>>> C = getcontext()
>>> C.add(Decimal(3), 'not a number')
NotImplemented

It's possible that a TypeError would make more sense here: NotImplemented should really only be returned by direct invocation of the double underscore magic methods (add, etc.).

Any patch should include tests in Lib/test/test_decimal.py, of course!

[jjconti]

The attached patch solves this issue.

Finally, only operand 'a' needs to be converted to Decimal if it's not. I discover this after writing my tests and start the implementation.

A Context.method is modified if it has more than one operand (for example a and b) and Decimal.method uses _convert_other

26 Context's methods were modified.

26 unit tets were added to ContextAPItests TestCase.

docstring was added to Context.divmod

[mdickinson]

Thanks for the patch!

Rather than using the Decimal constructor, I think you should convert use _convert_other(..., raiseit=True): the Decimal constructor converts strings and tuples, as well as ints and longs, while _convert_other only converts ints and longs. Note also that the conversion shouldn't depend on the current context; only the operation itself needs that.

Maybe it would be worth adding some tests to ensure that e.g.,

MyContext.add('4.5', 123)

raises TypeError as expected?

I agree with the observation that it's usually only necessary to convert the first argument (since the Decimal method itself converts the second).

If you like, you could also usefully deal with the NotImplemented return value by turning it into a TypeError (i.e., in the context method, check for a NotImplemented return value, and raise TypeError there if necessary). This is only needed for the double underscore methods __add__, __sub__, etc. associated with Python's binary operators; the other methods shouldn't ever return NotImplemented.

[mdickinson]

Thanks for the missing divmod docstring, too: I've applied this separately, partly because it needs to go into 2.6 and 3.1 as well as 2.7 and 3.2, and partly as an excuse to check that the new py3k-cdecimal branch is set up correctly. See revisions r77324 through r77327.

[jjconti]

New patch.

Fix Context.method that were returning NotImplemented.
Decimal.__methods__ don't use raiseit=True in _convert_other so I check in Context.method and raise TypeError if necessary.

Added tests for Context.divmod missed in first patch.

[mdickinson]

Thanks for the latest patch! It's looking good, but I have a few comments:

(1) It's not necessary to do an isinstance(a, Decimal) check before calling _convert_other, since _convert_other does that check anyway. It doesn't really harm either, but for consistency with the way the Decimal methods themselves are written, I think the isinstance check should be left out.

(2) The error message that's produced when the Decimal operation returns NotImplemented is a bit strange:

>>> decimal.getcontext().add(2, 'bob')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dickinsm/python/svn/trunk/Lib/decimal.py", line 3869, in add
    raise TypeError("Unable to convert %s to Decimal" % r)
TypeError: Unable to convert NotImplemented to Decimal

Presumably that '% r' should be '% b' instead.

(3) It looks like Context.power is missing a NotImplemented check:

>>> decimal.getcontext().power(2, 'bob')
NotImplemented

(4) We should consider updating the documentation for the Context methods, though there's actually nothing there that would suggest that these operations wouldn't work for integers.

[mdickinson]

One more:

(5) The patch includes a (presumably accidental) change to the divmod docstring, from "Return (a // b, a % b)" to "Return (self // other, self % other)". I think the first version is preferable.

[mdickinson]

And another. :)

(6) I think that with these changes, the single argument methods, like Context.exp, and Context.sqrt, should also accept integers. Thoughts?

[skrah]

(6) Yes, I think that is logical. In cdecimal, I accept integers for all unary methods except is_canonical and number_class.

[skrah]

If none of you is working on it right now, I'll produce a new patch.
Mark, how about this:

     def __add__(self, other, context=None, raiseit=False):
        """Returns self + other.

    -INF + INF (or the reverse) cause InvalidOperation errors.
    """
    other = _convert_other(other, raiseit)
    if other is NotImplemented:
        return other

Then the context functions could look cleaner.

[jjconti]

I've been working in the modified version of my last patch to solve the 6 mentioned points. I'm posting it in less than 24 hs.

If you're not hurry, please wait for me. This is just my second patch and is very useful to learn how to contribute to Python.

[skrah]

Juan: Sure, take your time. :) I just wanted to know if you were still busy with it.

[mdickinson]

Juan, don't worry about the documentation if you don't want to. I can fix that up easily. (Of course, if you do want to, go ahead!)

[facundobatista]

Juanjo, ping me in private if you want help with the doc toolchain, I can show you how to touch the .rst and see the changes after processing.

[jjconti]

1. Agree. Extra checks removed.
2. My mistake. Fixed.
3. Fexed.
4. Methods documentation fixed. Also added examples.
5. Fixed
6. Allow ints in the following unary methods (all except the ones excluded by skrah in cdecimal):

• abs
• canonical
• copy_abs
• copy_decimal
• copy_negate
• exp
• is_finite
• is_infinite
• is_nan
• is_normal
• is_qnan
• is_signed
• is_snan
• is_subnormal
• is_zero
• ln
• log10
• logb
• minus
• next_minus
• next_plus
• sqrt
• to_*_string
• to_integral_*

(also documented them properly as in 4)

copy_sing fixed and documented to have the same behaibour.

Ans a change in Doc/library/decimal.rst to reflec the new behaibour.

[mdickinson]

The updated patch looks good---thank you! We're getting there... :)

I'm not sure about the extra 'Operand can be Decimal or int.' in the method docstrings; this just looks like extra clutter to me. Rather, I think it would be a surprise worthy of documentation if these methods *didn't* accept int or long; since they now do (with your patch), I don't think it's really worth mentioning in the docstring. And it's not quite accurate, either, since these methods should accepts longs as well as ints (and bools, and instances of other subclasses).

Would you be content to remove these from the docstrings? Or do others monitoring this issue think they should stay?

The extra doctests and test_decimal tests are nice.

[mdickinson]

copy_sing fixed and documented to have the same behaibour.

Hmm. Thanks for noticing this: it looks like Decimal.copy_sign is missing a _convert_other call. I think that should be fixed in the Decimal class rather than in the Context class (so Context.copy_sign and Decimal.copy_sign should make one _convert_other call each, instead of Context.copy_sign making two calls).

[skrah]

I also think that the added docstrings are not really necessary.

Another thing: I forgot to mention 'canonical' in the list of functions
that should only accept Decimals. As with the other two (number_class
and is_canonical), this is a matter of taste. Personally I would not
expect them to work for integers.

[mdickinson]

Re: canonical. Yes, this made me pause for a second, too. But I don't see the harm in allowing it to accept ints and longs. Actually, it then provides a nice public version of _convert_other.

I'd probably also allow is_canonical and number_class to accept ints and longs, just on the basis that that gives fewer special cases.

[skrah]

Yes, indeed 'canonical' can be justified to take an integer, if we interpret the spec as:

'canonical' takes an operand and returns the preferred _decimal_
encoding of that operand.

But then 'is_canonical' should return false for an integer, and
this would create another special case: Accept an operand and
return something meaningful _without_ converting it first.

I think this is why I have problems with those two. 'number_class'
is less of a problem.

[jjconti]

Yeah... I did't like that docstring either :) Removed!
Also fixed Decimal.copy_sign, changed Context.copy_sign and added tests.

[mdickinson]

The latest patch looks fine. I've attached a slightly tweaked version:

• Add conversions for number_class; without this, number_class is inconsistent with the various is_*** methods. "c.is_normal(3)" should be equivalent to "c.number_class(3) in ('+Normal', '-Normal)", for example.

• Remove conversions for 'canonical' and 'is_canonical'; I agree with Stefan that these don't make a lot of sense. It's mostly academic, since I can't imagine either of these methods gets much use.

• I've reworded the documentation slightly.

• Remove lots of trailing whitespace (mostly on otherwise empty lines).

If this looks okay to everyone, I'll check it in.

[skrah]

It looks good (I agree on number_class), but I'd change these:

• Add raiseit=True to context.copy_decimal()

• Remove wrong comment from context.is_infinite()

• Add _convert_other(a, raiseit=True) to context.logical_invert()

• More whitespace in test_decimal()

The new tests could be condensed quite a bit by using getattr(), but
that isn't so important.

[mdickinson]

Thanks, Stefan. Applied to the trunk in r78217. I'll forward port to py3k.

[mdickinson]

Tweaked some doctests in r78218:

• add integer argument doctest for logical_invert
• fix integer literals with a leading zero for the other logical_***
  methods, since they're illegal in Python 3.

Merged to py3k in r78219.

Thanks, everyone!