Created on 2012-08-25 14:24 by ncoghlan, last changed 2012-12-16 20:26 by skrah. This issue is now closed.
|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|
|msg169135 - (view)||Author: Nick Coghlan (ncoghlan) *||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) *||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) *||Date: 2012-08-25 14:44|
|msg169139 - (view)||Author: Nick Coghlan (ncoghlan) *||Date: 2012-08-25 15:01|
While it's undocumented in the main docs , 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  http://docs.python.org/py3k/library/decimal#decimal.localcontext
|msg169140 - (view)||Author: Ezio Melotti (ezio.melotti) *||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) *||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) *||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) *||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) *||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) *||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) *||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) *||Date: 2012-08-28 10:28|
Nick Coghlan <email@example.com> 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) *||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) *||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) *||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) *||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) *||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) *||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) *||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) *||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) *||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) *||Date: 2012-12-16 20:26|
I think I've found all of them now. Closing.
|2012-12-16 20:26:43||skrah||set||status: open -> closed|
components: + Library (Lib)
versions: + Python 3.4
messages: + msg177626
stage: test needed -> committed/rejected
|2012-12-16 20:12:42||python-dev||set||messages: + msg177625|
title: decimal.localcontext(None) fails when using the C accelerator module -> decimal: Support None default values in the C accelerator module
messages: + msg177566
messages: + msg175735
messages: + msg175696
|2012-11-15 21:24:29||mark.dickinson||set||messages: + msg175647|
|2012-11-15 17:03:00||skrah||set||messages: + msg175628|
|2012-11-15 17:00:43||skrah||set||messages: + msg175627|
|2012-08-29 01:37:14||ncoghlan||set||messages: + msg169325|
|2012-08-28 20:43:14||mark.dickinson||set||messages: + msg169295|
|2012-08-28 19:55:31||skrah||set||messages: + msg169292|
|2012-08-28 10:28:41||skrah||set||messages: + msg169266|
|2012-08-28 06:13:11||ncoghlan||set||messages: + msg169253|
messages: + msg169146
|2012-08-25 17:30:48||mark.dickinson||set||messages: + msg169145|
|2012-08-25 17:25:07||skrah||set||messages: + msg169144|
|2012-08-25 16:10:03||skrah||set||messages: + msg169143|
|2012-08-25 15:56:38||skrah||set||messages: + msg169142|
nosy: + ezio.melotti
messages: + msg169140
keywords: + patch
|2012-08-25 15:01:10||ncoghlan||set||messages: + msg169139|
|2012-08-25 14:44:10||skrah||set||messages: + msg169138|
messages: + msg169136