[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Roundup Robot added the comment: 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 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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). -- Added file: http://bugs.python.org/file28009/issue15783-all-2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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. :) -- Added file: http://bugs.python.org/file28004/issue15783-all.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: By all cases I mean the ones listed in msg169144, not the Number methods. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Mark Dickinson added the comment: Should we go ahead with this? Sure: if you're willing to write the patch, go for it. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Nick Coghlan added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: Nick Coghlan rep...@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) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Mark Dickinson added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Nick Coghlan added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Changes by Zachary Ware zachary.w...@gmail.com: -- nosy: +zach.ware ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com: -- nosy: +Arfrever ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
New submission from Nick Coghlan: 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. -- keywords: 3.3regression messages: 169135 nosy: ncoghlan, skrah priority: normal severity: normal stage: test needed status: open title: decimal.localcontext(None) fails when using the C accelerator module type: behavior versions: Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Nick Coghlan added the comment: 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? -- nosy: +georg.brandl ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Nick Coghlan added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Ezio Melotti added the comment: 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. -- keywords: +patch nosy: +ezio.melotti Added file: http://bugs.python.org/file26995/issue15783.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Changes by Mark Dickinson dicki...@gmail.com: -- nosy: +mark.dickinson ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Stefan Krah added the comment: 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) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Mark Dickinson added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue15783] decimal.localcontext(None) fails when using the C accelerator module
Mark Dickinson added the comment: Adding Raymond to the nosy, since he may know more about what was intended. -- nosy: +rhettinger ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue15783 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
