classification
Title: decimal: Support None default values in the C accelerator module
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: skrah Nosy List: Arfrever, ezio.melotti, georg.brandl, mark.dickinson, ncoghlan, python-dev, rhettinger, skrah, zach.ware
Priority: normal Keywords: 3.3regression, patch

Created on 2012-08-25 14:24 by ncoghlan, last changed 2012-12-16 20:26 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
issue15783.diff ezio.melotti, 2012-08-25 15:04 review
issue15783-all.diff skrah, 2012-11-16 17:22 review
issue15783-all-2.diff skrah, 2012-11-17 12:40 review
Messages (24)
msg169135 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-25 14:24
decimal.localcontext() works correctly with both the pure Python decimal module and the C accelerator.

decimal.localcontext(None) only works with the pure Python version. The C accelerator throws an exception instead of treating it as equivalent to omitting the argument.
msg169136 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-25 14:27
Georg, a regression in the decimal API relative to 3.2 got picked up by #15136. Are you OK with cherrypicking a fix for this into rc2?
msg169138 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-25 14:44
The feature is undocumented though. Is it a regression?

There are other places as well. The topic was raised in msg153447 and
msg153506, with no further comments. So I concluded that None argument
support wasn't important.
msg169139 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-25 15:01
While it's undocumented in the main docs [1], help(decimal.localcontext) in 3.2 starts with:

localcontext(ctx=None)
    Return a context manager for a copy of the supplied context

We essentially have two votes in favour of "it should work" (the pure Python impl and the docstring), and two in favour of "meh" (the prose docs and the lack of a test case)

I think we should fix it, but I don't mind if Georg wants it to wait until 3.3.1

[1] http://docs.python.org/py3k/library/decimal#decimal.localcontext
msg169140 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-08-25 15:04
Can't this be fixed in the CONTEXT_CHECK_VA macro?
With the attached patch decimal.localcontext(None) works and there aren't any failures, however I don't think there are tests that pass None explicitly, so this approach might not work.
msg169142 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-25 15:56
OK, but for example:

Help on function __truediv__ in module decimal:

__truediv__(self, other, context=None)
    Return self / other.

Here I think it's undisputed that a C version should not cram a context
argument into a number method. There are many functions in decimal where
some arguments are just implementation details, or worse, leftovers
(see #10650).

So I simply don't know if localcontext(None) was intended or (once)
an implementation detail.

localcontext(None) by itself looks kind of awkward to me. The only valid
use case I can see is indeed the one from #15136.

I don't particularly mind either way for localcontext(), but in more
speed sensitive areas I would care.
msg169143 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-25 16:10
> Can't this be fixed in the CONTEXT_CHECK_VA macro?

No, the macro should only allow contexts. E.g. in the case of localcontext(None)
context_copy(local, NULL) would be called on local=Py_None, which is undefined.

What would perhaps be needed for a nice C-API is a function like

   PyArg_ParseTupleSkipNone()

that treats None arguments in the same way as optional args that are
not given.
msg169144 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-25 17:25
This is the complete list of context=None divergences. It would
be possible to change that, since most of the functions are
generated by macro wrappers.


# This is the only place where 'ctx' is used
localcontext(ctx=None)

canonical(self, context=None)
compare(self, other, context=None)
compare_signal(self, other, context=None)
exp(self, context=None)
fma(self, other, third, context=None)
is_normal(self, context=None)
is_subnormal(self, context=None)
ln(self, context=None)
log10(self, context=None)
logb(self, context=None)
logical_and(self, other, context=None)
logical_invert(self, context=None)
logical_or(self, other, context=None)
logical_xor(self, other, context=None)
max(self, other, context=None)
max_mag(self, other, context=None)
min(self, other, context=None)
min_mag(self, other, context=None)
next_minus(self, context=None)
next_plus(self, context=None)
normalize(self, context=None)
number_class(self, context=None)
quantize(self, exp, rounding=None, context=None, watchexp=True)
remainder_near(self, other, context=None)
rotate(self, other, context=None)
scaleb(self, other, context=None)
shift(self, other, context=None)
sqrt(self, context=None)
to_eng_string(self, context=None)
to_integral(self, rounding=None, context=None)
to_integral_value(self, rounding=None, context=None)
to_integral_exact(self, rounding=None, context=None)

context.power(self, a, b, modulo=None)
msg169145 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-08-25 17:30
This doesn't strike me as something it's essential to resolve before 3.3.1.

And I agree that decimal.localcontext(None) looks peculiar.  I noticed this yesterday when looking at #15136 and dismissed it as an artifact of the implementation---I didn't think to check what the docs said.  I think it's a bit unfortunate that the docs specify "ctx=None".  I doubt that too much thought went into that;  without knowing the history, I suspect that at the time it was just a way to indicate that the context was an optional argument.

IOW, my vote would be not to require that None is accepted, and to treat this as an minor error in the documentation.
msg169146 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-08-25 17:31
Adding Raymond to the nosy, since he may know more about what was intended.
msg169253 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-28 06:13
Note that it's only the 3.2 interactive help that specifies "ctx=None", and it's almost certainly lifting that directly from the Python implementation.

The C implementation is definitely in compliance with the prose docs, which give the signature as localcontext([c]) and say "If no context is specified, a copy of the current context is used" without indicating whether passing in None explicitly is supposed to count as not specifying a context.

However, the specific problem that prompted this report is that you used to be able to write code like:

  def my_decimal_func(arg1, arg2, context=None):
    with decimal.localcontext(context):
        # perform decimal operations on arg1 and arg2

but with the C implementation, such code will break. You have to instead write something less idiomatic like:

  def my_decimal_func(arg1, arg2, *context):
    with decimal.localcontext(*context):
        # perform decimal operations on arg1 and arg2

Any third party Decimal manipulating function that accepts an optional context and passes it down to the standard Decimal API will be confronted with the same problem in 3.3: passing None as the context no longer means the same thing as omitting the context argument entirely.

We can either handle this as a documentation issue (recommending the *args based idiom above as an alternative to passing an explicit None), or else change the C implementation to accept context=None as equivalent to omitting the context argument entirely.

I'm inclined towards the latter, simply due to the backwards compatibility breach: explicitly calling localcontext(None) would indeed be silly, but calling localcontext(context), where context comes from a parameter with a default value of None, is perfectly normal Python code. Idiomatic code that worked correctly in the previous Python version is a reasonably strong argument in favour of retaining the feature.
msg169266 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-28 10:28
Nick Coghlan <report@bugs.python.org> wrote:
> Any third party Decimal manipulating function that accepts an
> optional context and passes it down to the standard Decimal API
> will be confronted with the same problem in 3.3: passing None
> as the context no longer means the same thing as omitting the
> context argument entirely.

I agree, but for me the issue is: What is the standard API?

If you look at the Context methods in decimal.py, you see this pattern:

def add(self, a, b):
    ...
    r = a.__add__(b, context=self)


So I think it's reasonably safe to say that all Decimal methods only
take a context=None argument because it happens to be a convenient way
of implementing the Context methods.

With that reasoning, most of the list in msg169144 would be eliminated
already. I sort of regret that the Decimal methods of the C version take
a context argument at all, but the arguments are documented.

Now to localcontext(ctx=None). Yes, code might exist that does the
following:

def f(a, b, context=None):
    with localcontext(context):
        return a / b


It is, however, a strange function: If I explicitly pass a context to
a function, I'd expect that it is also used for recording any status
that accumulates in the function (or that the function actually *can*
accumulate status at all).


If I'm only interested in the precision, I'd write:

def f(a, b, prec=None):
    with localcontext() as c:
        c.prec = 9 if prec is None else prec
        return Decimal(a) / b


[This is along the lines of Raymond's original suggestion in #15136.]


But there are other examples of unexpected behavior, such as
Decimal.to_eng_string(context) taking a context purely to determine
the value of context.capitals, i.e. no status flags can possibly be
set. Here I'd also prefer:

    to_eng_string(capitals=1)
msg169292 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-28 19:55
I don't want to block this, BTW. Personally I'm +-0 on the issue.

If more people think this is needed for backwards compatibility, I'll
write a patch for localcontext. But I'd rather do that *very* soon.

FWIW, none of the cdecimal users has ever complained about this.

So we have a tie here: Mark thinks it can wait, Nick thinks it's
better to do something.
msg169295 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-08-28 20:43
I don't really feel that strongly either way, either.  I understand Nick's arguments, but can't help feeling that we're letting a doc mistake dictate the API here.  (And my inner type-system has an aversion to APIs where arguments are expected to be *either* None or of type/interface XXX.)

So colour me -0 on changing cdecimal to allow the 'None' argument.
msg169325 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-08-29 01:37
I agree it's an acceptable risk for 3.3.0. Most third party decimal operations simply won't accept an explicit context argument at all - avoiding the need to pass the current context around explicitly is the whole point of it being stored in the thread locals.

However, I still think it would be better if consistency with the Python API could be restored for 3.3.1. If that's done via a more general "this value means the arg is actually missing" C API that can be made public in 3.4, so much the better.
msg175627 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-11-15 17:00
Should we go ahead with this? If so, then I think it only makes sense
to change *all* undocumented cases of None default args. Here's a
case that used rounding=None in quantize():

https://bitbucket.org/simplecodes/wtforms/issue/117/test-failure-with-python-33
msg175628 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-11-15 17:03
By "all cases" I mean the ones listed in msg169144, not the Number methods.
msg175647 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-15 21:24
> Should we go ahead with this?

Sure:  if you're willing to write the patch, go for it.
msg175696 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-11-16 17:22
OK, here's a patch. Specifics:

  o Except for the number methods, decimal.py and _decimal should behave
    identically now.

  o _decimal actually requires an additional context arg in same_quantum(),
    compare_total(), compare_total_mag() and copy_sign(). This is for raising
    InvalidOperation if the second operand is a very large integer and cannot
    be converted exactly.

    I've added the context arguments to the Python version to keep compatibility.

  o For some reason decimal.py uses a context arg in canonical(), so I've
    added it to the C version.

  o localcontext(ctx=None) is pretty ugly. Perhaps we should make the
    backwards incompatible switch to localcontext(context=None). I don't
    care a lot though.

  o I'd like to update the docstrings and the documentation in a separate
    patch.

  o The important idiom c.power(modulo=None, b=9, a=3) is now supported. :)
msg175735 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-11-17 12:40
New patch with docs. I've eliminated the unused context arg from canonical(). I can't possibly imagine that anyone uses it (or
that anyone uses canonical() at all for that matter).
msg177566 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-15 21:40
New changeset 29becac5dd9b by Stefan Krah in branch '3.3':
Issue #15783: Except for the number methods, the C version of decimal now
http://hg.python.org/cpython/rev/29becac5dd9b
msg177567 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-12-15 21:51
Ezio, thanks for the Rietveld comments.


There are still unsupported None default values in the Context
constructor, so I'm leaving the issue open.
msg177625 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-16 20:12
New changeset 907d71668d3c by Stefan Krah in branch '3.3':
Issue #15783: Support None default values in the Context() constructor.
http://hg.python.org/cpython/rev/907d71668d3c
msg177626 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-12-16 20:26
I think I've found all of them now. Closing.
History
Date User Action Args
2012-12-16 20:26:43skrahsetstatus: open -> closed

assignee: skrah
components: + Library (Lib)
versions: + Python 3.4
resolution: fixed
messages: + msg177626
stage: test needed -> resolved
2012-12-16 20:12:42python-devsetmessages: + msg177625
2012-12-15 21:51:09skrahsetmessages: + msg177567
title: decimal.localcontext(None) fails when using the C accelerator module -> decimal: Support None default values in the C accelerator module
2012-12-15 21:40:30python-devsetnosy: + python-dev
messages: + msg177566
2012-11-17 12:40:05skrahsetfiles: + issue15783-all-2.diff

messages: + msg175735
2012-11-16 17:22:18skrahsetfiles: + issue15783-all.diff

messages: + msg175696
2012-11-15 21:24:29mark.dickinsonsetmessages: + msg175647
2012-11-15 17:03:00skrahsetmessages: + msg175628
2012-11-15 17:00:43skrahsetmessages: + msg175627
2012-08-29 01:37:14ncoghlansetmessages: + msg169325
2012-08-28 20:43:14mark.dickinsonsetmessages: + msg169295
2012-08-28 19:55:31skrahsetmessages: + msg169292
2012-08-28 10:28:41skrahsetmessages: + msg169266
2012-08-28 06:13:11ncoghlansetmessages: + msg169253
2012-08-27 16:29:26zach.waresetnosy: + zach.ware
2012-08-26 20:05:03Arfreversetnosy: + Arfrever
2012-08-25 17:31:56mark.dickinsonsetnosy: + rhettinger
messages: + msg169146
2012-08-25 17:30:48mark.dickinsonsetmessages: + msg169145
2012-08-25 17:25:07skrahsetmessages: + msg169144
2012-08-25 16:30:07mark.dickinsonsetnosy: + mark.dickinson
2012-08-25 16:10:03skrahsetmessages: + msg169143
2012-08-25 15:56:38skrahsetmessages: + msg169142
2012-08-25 15:04:16ezio.melottisetfiles: + issue15783.diff

nosy: + ezio.melotti
messages: + msg169140

keywords: + patch
2012-08-25 15:01:10ncoghlansetmessages: + msg169139
2012-08-25 14:44:10skrahsetmessages: + msg169138
2012-08-25 14:27:58ncoghlansetnosy: + georg.brandl
messages: + msg169136
2012-08-25 14:24:43ncoghlancreate