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 constants should be the same for py & c module versions #60626

Closed
1st1 opened this issue Nov 6, 2012 · 12 comments
Closed

Decimal constants should be the same for py & c module versions #60626

1st1 opened this issue Nov 6, 2012 · 12 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@1st1
Copy link
Member

1st1 commented Nov 6, 2012

BPO 16422
Nosy @mdickinson, @asvetlov, @skrah, @1st1
Files
  • issue16422.diff
  • 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/skrah'
    closed_at = <Date 2013-01-16.12:15:42.979>
    created_at = <Date 2012-11-06.23:04:32.504>
    labels = ['extension-modules', 'type-bug']
    title = 'Decimal constants should be the same for py & c module versions'
    updated_at = <Date 2013-01-16.12:15:42.977>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2013-01-16.12:15:42.977>
    actor = 'skrah'
    assignee = 'skrah'
    closed = True
    closed_date = <Date 2013-01-16.12:15:42.979>
    closer = 'skrah'
    components = ['Extension Modules']
    creation = <Date 2012-11-06.23:04:32.504>
    creator = 'yselivanov'
    dependencies = []
    files = ['28736']
    hgrepos = []
    issue_num = 16422
    keywords = ['patch']
    message_count = 12.0
    messages = ['175024', '175027', '175029', '175030', '175032', '175035', '175036', '175058', '175122', '180013', '180080', '180085']
    nosy_count = 5.0
    nosy_names = ['mark.dickinson', 'asvetlov', 'skrah', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16422'
    versions = ['Python 3.3', 'Python 3.4']

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 6, 2012

    Right now decimal.py defines 'ROUND_DOWN' as 'ROUND_DOWN' (string), whereas C version define it as 1 (integer).

    While using constant values directly in your code is not a good idea, there is another case where it doesn't work: if you serialize decimal values along with their meta information, such as rounding settings. In this case, when you unserialize data that was serialized with python decimal, those meta-settings won't work for 'c' decimal.

    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label Nov 6, 2012
    @mdickinson
    Copy link
    Member

    I think it's already true that pickling a Decimal context on Python 3.2 and unpickling on Python 3.3 doesn't work. Stefan: do I recall that this is a known issue?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 6, 2012

    Pickling changed in 3.3 to make the C and Python versions compatible. So pickled
    contexts in 3.3 are actually interchangeable.

    Pickling a list of rounding modes is not compatible.

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 6, 2012

    Well, I don't care about py 3.2 & 3.3 pickle compatibility in this particular issue. This one is about compatibility of py & c decimal modules in 3.3.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 6, 2012

    So what data structure are you trying to serialize interchangeably?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 6, 2012

    I see that you mentioned the use case in your first mail. Yes, that isn't
    possible right now.

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 6, 2012

    Right ;)

    Is there any chance we can fix that in next 3.3 point release or 3.4?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 7, 2012

    It would be possible to translate strings to integers; the infrastructure
    is already there for pickling. The decision not to do that was actually
    deliberate: Up to now no one has requested string constants for rounding
    modes and I *suspect* that there's a performance penalty even though I
    didn't measure it.

    See Modules/_decimal/_decimal.c:1211 for the code that would need to
    be called each time something like this occurs:

      context.rounding = ROUND_UP

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Nov 7, 2012

    Well, I suppose one could use a cascaded switch statement, starting with the
    7th letter and then (in the case of HALF*) proceed with the 12th letter:

    "ROUND_CEILING"
    "ROUND_FLOOR"
    "ROUND_UP"
    "ROUND_DOWN"
    "ROUND_HALF_UP"
    "ROUND_HALF_DOWN"
    "ROUND_HALF_EVEN"
    "ROUND_05UP"

    That should be as fast as PyLong_AsLong().

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 15, 2013

    In the absence of an enum type, string constants are nicer to read in
    the REPL, so here's a patch. Translating between strings and ints is
    fast if you use the module constants.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 16, 2013

    New changeset 733bb4fd9888 by Stefan Krah in branch '3.3':
    Issue bpo-16422: Use strings for rounding mode constants for better readability
    http://hg.python.org/cpython/rev/733bb4fd9888

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 16, 2013

    In the version I committed the string constants are interned, so
    anything but legacy strings should be reasonably fast.

    @skrah skrah mannequin added the extension-modules C modules in the Modules dir label Jan 16, 2013
    @skrah skrah mannequin closed this as completed Jan 16, 2013
    @skrah skrah mannequin self-assigned this Jan 16, 2013
    @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
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants