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

Make Context._clamp public in decimal module #52786

Closed
mdickinson opened this issue Apr 26, 2010 · 15 comments
Closed

Make Context._clamp public in decimal module #52786

mdickinson opened this issue Apr 26, 2010 · 15 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@mdickinson
Copy link
Member

BPO 8540
Nosy @rhettinger, @facundobatista, @mdickinson, @skrah
Files
  • decimal_public_clamp.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-05-22.18:36:28.747>
    created_at = <Date 2010-04-26.17:45:30.022>
    labels = ['type-feature', 'library']
    title = 'Make Context._clamp public in decimal module'
    updated_at = <Date 2011-10-24.09:33:52.040>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2011-10-24.09:33:52.040>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2010-05-22.18:36:28.747>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2010-04-26.17:45:30.022>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['17438']
    hgrepos = []
    issue_num = 8540
    keywords = ['patch']
    message_count = 15.0
    messages = ['104263', '104306', '104307', '104308', '104310', '104322', '104344', '104504', '106290', '106292', '106293', '106300', '106305', '106320', '146285']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'skrah']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'test needed'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8540'
    versions = ['Python 3.2']

    @mdickinson
    Copy link
    Member Author

    The Context class in the decimal module has a hidden _clamp attribute, that controls whether to clamp values with large exponents. (Clamping a Decimal value involves adding extra significant zeros to decrease its exponent, while not altering the numeric value; it's sometimes necessary to get the exponent within a legal range).

    I think we should consider making this attribute public (documenting it, having it appear in the __str__ or __repr__ of a Context), for a couple of reasons:

    (1) It's necessary for full compliance with the specification: Python's default behaviour is actually non-compliant with the spec; it's only after doing getcontext()._clamp = 1 that it complies.

    (2) Clamping is necessary for modeling the standard formats described in IEEE 754-2008 (decimal64, decimal128), etc. These formats are coming into use in other languages (gcc already supports them and C201x may well include them). To be able to communicate effectively with other languages using these formats, it would be useful to expose Context._clamp.

    @mdickinson mdickinson added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Apr 26, 2010
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 27, 2010

    (1) Perhaps I missed the relevant part in the spec, so I had to check
    what decNumber does: In the default context, clamp is 0, in the extended
    contexts, it is 1. So Python's ExtendedContext should indeed set _clamp
    to 1.

    (2) I agree about the importance of the formats, and I think they should
    be explicitly selectable like here:

    http://java.sun.com/j2se/1.5.0/docs/api/java/math/class-use/MathContext.html

    The current ExtendedContext is a bit of a vague concept, since it
    should be exactly one of DECIMAL32, DECIMAL64 or DECIMAL128. And
    none of these has prec=9.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 27, 2010

    I knew it was somewhere: In the test case description, clamp=0 is
    specified as the default:

    http://speleotrove.com/decimal/dtfile.html#syntax

    @mdickinson
    Copy link
    Member Author

    Hmm. Yes, the spec itself is rather vague on the subject of clamping, so I withdraw my claim that clamping is necessary for compliance with the spec. It *is* necessary to make the those testcases with 'clamp=1' pass, though.

    So from the point of view of compliance with the specification it's fine to leave the _clamp attribute private.

    I agree it would be useful to give easy access to the IEEE 754 formats, though, for interoperability with C and Java (thanks for the Java link!). The most important formats are decimal32, decimal64 and decimal128, but IEEE 754 also specifies parameters for decimal{k} where k is any multiple of 32 bits.

    @mdickinson
    Copy link
    Member Author

    Re: ExtendedContext, the comments in decimal.py say:

    # Pre-made alternate contexts offered by the specification
    # Don't change these; the user should be able to select these
    # contexts and be able to reproduce results from other implementations
    # of the spec.

    This doesn't make a lot of sense to me, since (as Stefan says) the choice of precision 9 doesn't seem to come from the specification.

    However, the current ExtendedContext is used extensively in doctests and elsewhere; it would be awkward to change it now. I think adding the IEEE formats and encouraging users to use those in place of ExtendedContext might be the best bet.

    I'd also still want to make _clamp public in this case, to avoid people wondering why two apparently identical contexts (one with _clamp=1, one with _clamp=0) can lead to different results.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 27, 2010

    I'd prefer to drop the ExtendedContext completely. Reasons are:

    1. _clamp, prec, emin and emax aren't set to IEEE754 values.

    2. The use of 'extended' is decNumber specific (see
      http://speleotrove.com/decimal/dncont.html ). In IEEE754
      'extended' has yet another meaning (AFAICS).

    I can see that it is awkward to remove it, but if there's consensus
    I'd be willing to work on a patch.

    Making clamp visible sounds fine to me. (Personally, I'd rather have
    capitals non-visible.)

    If we make Decimal{32,64,128} contexts available, we should document exactly how the arithmetic deviates from IEEE754.

    @mdickinson
    Copy link
    Member Author

    I'd prefer to drop the ExtendedContext completely.

    We have to be careful not to break existing 3rd party code, though. A newly-designed decimal module, prepared with 20/20 hindsight, probably wouldn't include an ExtendedContext with the current definition. But we're not dealing with a newly-designed module: we're dealing with a well-established and well-used module, so there's not a lot of scope for removals or major behaviour changes.

    Personally, I don't think the design error (if there is a design error here) is big enough to make it worth breaking backwards compatibility. I'd rather leave ExtendedContext in for the duration of Python 3.x, but introduce better options and steer (via the documentation) new decimal users towards those.

    (BTW, Python takes backwards compatibility pretty seriously: see PEP-387 for a write-up of the policy.)

    If we make Decimal{32,64,128} contexts available, we should document exactly how the arithmetic deviates from IEEE754.

    Possibly, though that's less important to me than just being able to read and write values in these formats produced by other systems.

    Were you thinking of any deviations in particular?

    Making clamp public should be straightforward enough though, and is independent of the changes discussed above; I'll see if I can come up with a patch. (Even here, though, I think it makes sense to leave the private _clamp attribute in place in case people are using it; a new public 'clamp' property can be added that wraps the _clamp attribute.)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 29, 2010

    Were you thinking of any deviations in particular?

    More or less a commentary on what is listed here:

    http://speleotrove.com/decimal/dascope.html

    ==> Restrictions

    I only have a draft of IEEE754, but for instance I can't find a power
    or remainder-near function there.

    @mdickinson mdickinson self-assigned this Apr 30, 2010
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 22, 2010

    I'm busy implementing the IEEE754 contexts for cdecimal. To keep things
    in sync, it would be nice to agree how they should be created.

    Suggestions:

    1. c = Decimal64Context

    2. c = Context(Decimal64)

    3. ?

    I have a preference for 2). It's clear that you get a new Object and
    the user does not have to wonder if a template context will be
    contaminated when using setcontext(Decimal64Context).

    (I know there are measures against that, but setcontext( Context(Decimal64))
    is explicit rather than implicit.)

    @mdickinson
    Copy link
    Member Author

    1. c = Decimal64Context

    2. c = Context(Decimal64)

    Rather that complicating the Context constructor, I'd prefer a separate factory function. I was thinking of something like:

    def IEEEContext(n):
        """Return the decimal<n> IEEE 754 context.  n should be a multiple
        of 32."""
        ...

    Again, it's clear with this that you get a new context object (I agree that there are problems with (1) and the mutability of Contexts).

    @mdickinson
    Copy link
    Member Author

    BTW, let's open another issue for support of the IEEE 754 contexts, and keep this one for exposing _clamp. Otherwise life gets confusing when you're trying to decide when an issue can be closed, etc...

    @mdickinson
    Copy link
    Member Author

    Here's a patch for changing '_clamp' into 'clamp'.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 22, 2010

    The patch looks good, +1 for applying it. I'm not a native speaker, but
    probably:

    "are subject to clamping this manner" => "are subject to clamping in this manner"

    @mdickinson
    Copy link
    Member Author

    Thanks for catching the doc typo! Actually, I didn't like that doc addition anyway, so I rewrote it.

    Committed in r81476.

    @mdickinson
    Copy link
    Member Author

    New changeset 221638ba5d2a by Mark Dickinson in branch 'default':
    Issue bpo-13248, issue bpo-8540: Remove deprecated Context._clamp attribute from Decimal module.
    http://hg.python.org/cpython/rev/221638ba5d2a

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant