classification
Title: decimal.py: type conversion in context methods
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: facundobatista, jjconti, mark.dickinson, rhettinger, skrah
Priority: normal Keywords: easy, patch

Created on 2010-01-04 13:01 by skrah, last changed 2010-02-18 14:53 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
issue7633_jjconti.patch jjconti, 2010-01-06 01:47 Patch from Juanjo Conti for issue 7633
issue7633_jjconti2.patch jjconti, 2010-01-06 19:13 New Patch for issue 7633
issue7633_jjconti3.patch jjconti, 2010-01-29 02:20 3rd patch
issue7633_jjconti4.patch jjconti, 2010-01-30 00:45 4th patch
issue7633_jjconti4_metd.patch mark.dickinson, 2010-02-17 16:30
issue7633_jjconti5_metd.patch skrah, 2010-02-18 13:00
Messages (29)
msg97207 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-04 13:01
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.
msg97259 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-05 11:20
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
msg97260 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-05 12:53
I agree that this would be desirable.  And yes, c.add(9, 8) should be allowed, too.

Anyone interested in producing a patch?
msg97261 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-05 14:01
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)
?
msg97268 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-05 17:49
> 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!
msg97288 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-06 01:47
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
msg97304 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-06 10:53
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.
msg97308 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-06 16:26
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.
msg97317 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-06 19:13
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.
msg98212 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-24 10:52
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.
msg98213 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-24 10:56
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.
msg98214 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-24 11:00
And another.  :)

(6) I think that with these changes, the single argument methods, like Context.exp, and Context.sqrt, should also accept integers.  Thoughts?
msg98216 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-24 11:37
(6) Yes, I think that is logical. In cdecimal, I accept integers for all unary methods except is_canonical and number_class.
msg98346 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-26 18:41
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.
msg98347 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-26 18:44
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.
msg98349 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-26 18:52
Juan: Sure, take your time. :) I just wanted to know if you were still busy with it.
msg98351 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-26 18:58
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!)
msg98354 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2010-01-26 19:08
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.
msg98497 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-29 02:20
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.
msg98526 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-29 19:03
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.
msg98527 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-29 19:11
> 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).
msg98534 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-29 20:22
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.
msg98535 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-01-29 20:25
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.
msg98541 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-01-29 21:47
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.
msg98544 - (view) Author: Juan José Conti (jjconti) * Date: 2010-01-30 00:45
Yeah... I did't like that docstring either :) Removed!
Also fixed Decimal.copy_sign, changed Context.copy_sign and added tests.
msg99475 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-02-17 16:30
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.
msg99502 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-02-18 13:00
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.
msg99507 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-02-18 14:28
Thanks, Stefan.  Applied to the trunk in r78217.  I'll forward port to py3k.
msg99509 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-02-18 14:53
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!
History
Date User Action Args
2010-02-18 14:53:06mark.dickinsonsetstatus: open -> closed
resolution: accepted
messages: + msg99509

stage: needs patch -> resolved
2010-02-18 14:28:22mark.dickinsonsetmessages: + msg99507
2010-02-18 13:00:10skrahsetfiles: + issue7633_jjconti5_metd.patch

messages: + msg99502
2010-02-17 16:30:43mark.dickinsonsetfiles: + issue7633_jjconti4_metd.patch

messages: + msg99475
2010-01-30 00:45:18jjcontisetfiles: + issue7633_jjconti4.patch

messages: + msg98544
2010-01-29 21:47:09skrahsetmessages: + msg98541
2010-01-29 20:25:16mark.dickinsonsetmessages: + msg98535
2010-01-29 20:22:23skrahsetmessages: + msg98534
2010-01-29 19:11:23mark.dickinsonsetmessages: + msg98527
2010-01-29 19:03:38mark.dickinsonsetmessages: + msg98526
2010-01-29 02:20:54jjcontisetfiles: + issue7633_jjconti3.patch

messages: + msg98497
2010-01-26 19:08:39facundobatistasetmessages: + msg98354
2010-01-26 18:58:17mark.dickinsonsetmessages: + msg98351
2010-01-26 18:52:51skrahsetmessages: + msg98349
2010-01-26 18:44:57jjcontisetmessages: + msg98347
2010-01-26 18:41:28skrahsetmessages: + msg98346
2010-01-24 11:37:55skrahsetmessages: + msg98216
2010-01-24 11:00:18mark.dickinsonsetmessages: + msg98214
2010-01-24 10:56:31mark.dickinsonsetmessages: + msg98213
2010-01-24 10:52:47mark.dickinsonsetmessages: + msg98212
2010-01-11 12:10:05mark.dickinsonsetassignee: mark.dickinson
2010-01-06 19:13:55jjcontisetfiles: + issue7633_jjconti2.patch

messages: + msg97317
2010-01-06 16:26:21mark.dickinsonsetmessages: + msg97308
2010-01-06 10:53:40mark.dickinsonsetmessages: + msg97304
2010-01-06 01:47:22jjcontisetfiles: + issue7633_jjconti.patch
type: behavior
messages: + msg97288

keywords: + patch
2010-01-05 17:49:22mark.dickinsonsetmessages: + msg97268
2010-01-05 14:01:05jjcontisettype: enhancement -> (no value)
messages: + msg97261
2010-01-05 12:53:34mark.dickinsonsetpriority: normal

type: enhancement
components: + Library (Lib)
versions: + Python 2.7, Python 3.2
keywords: + easy
nosy: + rhettinger, facundobatista

messages: + msg97260
stage: needs patch
2010-01-05 11:20:38jjcontisetnosy: + jjconti
messages: + msg97259
2010-01-04 13:01:12skrahcreate