Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decimal.py: type conversion in context methods #51882

Closed
skrah mannequin opened this issue Jan 4, 2010 · 29 comments
Closed

decimal.py: type conversion in context methods #51882

skrah mannequin opened this issue Jan 4, 2010 · 29 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@skrah
Copy link
Mannequin

skrah mannequin commented Jan 4, 2010

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
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 4, 2010

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 5, 2010

    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
    Copy link
    Member

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

    Anyone interested in producing a patch?

    @mdickinson mdickinson added stdlib Python modules in the Lib dir easy type-feature A feature request or enhancement labels Jan 5, 2010
    @jjconti
    Copy link
    Mannequin

    jjconti mannequin commented Jan 5, 2010

    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)
    

    ?

    @jjconti jjconti mannequin removed the type-feature A feature request or enhancement label Jan 5, 2010
    @mdickinson
    Copy link
    Member

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 6, 2010

    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

    @jjconti jjconti mannequin added the type-bug An unexpected behavior, bug, or error label Jan 6, 2010
    @mdickinson
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 6, 2010

    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 mdickinson self-assigned this Jan 11, 2010
    @mdickinson
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 24, 2010

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

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 26, 2010

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 26, 2010

    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
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 26, 2010

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

    @mdickinson
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 29, 2010

    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
    Copy link
    Member

    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 link
    Member

    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
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 29, 2010

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    skrah mannequin commented Jan 29, 2010

    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
    Copy link
    Mannequin

    jjconti mannequin commented Jan 30, 2010

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

    @mdickinson
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    skrah mannequin commented Feb 18, 2010

    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
    Copy link
    Member

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

    @mdickinson
    Copy link
    Member

    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!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants