Skip to content

gh-73487: Convert _decimal to use Argument Clinic (part 1) #137606

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Aug 10, 2025

@skirpichev skirpichev changed the title gh-73487: convert the decimal module to use AC gh-73487: convert the _decimal module to use AC Aug 10, 2025
@skirpichev

This comment has been minimized.

@AA-Turner
Copy link
Member

AA-Turner commented Aug 10, 2025

Is the help(decimal) output identical before and after the PR?

I appreciate suggestion on how to make it easier to review.

It looks worse than it actually is, only ~400 lines diff in _decimal.c and ~190 lines of docstring-only changes in _decimal/docstrings.h. You could maybe split up the Decimal methods into a few groups to reduce the size of the PR, but no obvious such groupings spring to mind.

@skirpichev
Copy link
Contributor Author

It looks worse than it actually is

As I said, it's more or less a mechanical change. No changes in signatures, docstrings are copied from docstrings.h (though, AC enforces to have PEP summary line, most decimal docstrings don't follow this - that changed). There should be a minor speedup from using METH_FASTCALL (see issue thread for examples, I'll do benchmarks later, maybe add news). I'm planning to use also METH_METHOD where possible, but in a separate patch.

no obvious such groupings spring to mind.

Different types, then method functions? I don't see how to make such split uniform.

@AA-Turner

This comment was marked as outdated.

AA-Turner

This comment was marked as outdated.

@AA-Turner AA-Turner changed the title gh-73487: convert the _decimal module to use AC gh-73487: Convert _decimal to use Argument Clinic Aug 10, 2025
skirpichev and others added 2 commits August 11, 2025 04:09
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@AA-Turner

This comment was marked as resolved.

@skirpichev

This comment was marked as resolved.

@AA-Turner
Copy link
Member

AA-Turner commented Aug 11, 2025

Do you think it's now too hard to review?

Yes. I'm happy to look at the changes, just in a future PR instead of this one.

This reverts commit 354d8db.
@skirpichev skirpichev requested a review from AA-Turner August 11, 2025 02:08
@AA-Turner

This comment was marked as resolved.

AA-Turner

This comment was marked as resolved.

@skirpichev skirpichev changed the title gh-73487: Convert _decimal to use Argument Clinic gh-73487: Convert _decimal to use Argument Clinic (part 1) Aug 11, 2025
@skirpichev

This comment was marked as outdated.

@skirpichev
Copy link
Contributor Author

Ok, I did some final updates an added news. @AA-Turner, please review.

I asked also @rhettinger and @serhiy-storchaka for review, as previous module maintainer was against AC change (though, I think he was fine with using METH_FASTCALL).

Simple benchmark:

Benchmark ref patch
a.copy_sign(b) 678 ns 447 ns: 1.52x faster
a.to_integral(rnd, ctx) 688 ns 449 ns: 1.53x faster
Geometric mean (ref) 1.52x faster
# bench.py
import pyperf
from decimal import Decimal as D
from decimal import ROUND_HALF_EVEN as rnd, getcontext

a, b = map(D, [1, 2])
ctx = getcontext()

runner = pyperf.Runner()
runner.bench_func('a.copy_sign(b)', a.copy_sign, b)
runner.bench_func('a.to_integral(rnd, ctx)', a.to_integral, rnd, ctx)

pydoc diff:

--- ref.txt     2025-08-11 11:04:08.585913041 +0300
+++ patch.txt   2025-08-11 10:46:13.043516864 +0300
@@ -567,19 +567,19 @@
      |      Return the adjusted exponent of the number.  Defined as exp + digits - 1.
      |
      |  as_integer_ratio(self, /)
-     |      Decimal.as_integer_ratio() -> (int, int)
+     |      Return a pair of integers whose ratio is exactly equal to the original.
      |
-     |      Return a pair of integers, whose ratio is exactly equal to the original
-     |      Decimal and with a positive denominator. The ratio is in lowest terms.
+     |      The ratio is in lowest terms and with a positive denominator.
      |      Raise OverflowError on infinities and a ValueError on NaNs.
      |
      |  as_tuple(self, /)
      |      Return a tuple representation of the number.
      |
      |  canonical(self, /)
-     |      Return the canonical encoding of the argument.  Currently, the encoding
-     |      of a Decimal instance is always canonical, so this operation returns its
-     |      argument unchanged.
+     |      Return the canonical encoding of the argument.
+     |
+     |      Currently, the encoding of a Decimal instance is always canonical, so this
+     |      operation returns its argument unchanged.
      |
      |  compare(self, /, other, context=None)
      |      Compare self to other.  Return a decimal value:
@@ -626,16 +626,21 @@
      |      Return self.
      |
      |  copy_abs(self, /)
-     |      Return the absolute value of the argument.  This operation is unaffected by
-     |      context and is quiet: no flags are changed and no rounding is performed.
+     |      Return the absolute value of the argument.
+     |
+     |      This operation is unaffected by context and is quiet: no flags are changed and
+     |      no rounding is performed.
      |
      |  copy_negate(self, /)
-     |      Return the negation of the argument.  This operation is unaffected by context
-     |      and is quiet: no flags are changed and no rounding is performed.
+     |      Return the negation of the argument.
+     |
+     |      This operation is unaffected by context and is quiet: no flags are changed and
+     |      no rounding is performed.
      |
      |  copy_sign(self, /, other, context=None)
-     |      Return a copy of the first operand with the sign set to be the same as the
-     |      sign of the second operand. For example:
+     |      Return a copy of *self* with the sign set to be the same as the sign of *other*.
+     |
+     |      For example:
      |
      |          >>> Decimal('2.3').copy_sign(Decimal('-1.5'))
      |          Decimal('-2.3')
@@ -763,8 +768,9 @@
      |      to the equivalent value Decimal('32.1').
      |
      |  number_class(self, /, context=None)
-     |      Return a string describing the class of the operand.  The returned value
-     |      is one of the following ten strings:
+     |      Return a string describing the class of the operand.
+     |
+     |      The returned value is one of the following ten strings:
      |
      |          * '-Infinity', indicating that the operand is negative infinity.
      |          * '-Normal', indicating that the operand is a negative normal number.
@@ -778,8 +784,7 @@
      |          * 'sNaN', indicating that the operand is a signaling NaN.
      |
      |  quantize(self, /, exp, rounding=None, context=None)
-     |      Return a value equal to the first operand after rounding and having the
-     |      exponent of the second operand.
+     |      Return a value equal to *self* after rounding, with the exponent of *other*.
      |
      |          >>> Decimal('1.41421356').quantize(Decimal('1.000'))
      |          Decimal('1.414')
@@ -798,7 +803,9 @@
      |      argument is given, the rounding mode of the current thread's context is used.
      |
      |  radix(self, /)
-     |      Return Decimal(10), the radix (base) in which the Decimal class does
+     |      Return Decimal(10).
+     |
+     |      This is the radix (base) in which the Decimal class does
      |      all its arithmetic. Included for compatibility with the specification.
      |
      |  remainder_near(self, /, other, context=None)
@@ -846,34 +853,41 @@
      |      correctly rounded using the ROUND_HALF_EVEN rounding mode.
      |
      |  to_eng_string(self, /, context=None)
-     |      Convert to an engineering-type string.  Engineering notation has an exponent
-     |      which is a multiple of 3, so there are up to 3 digits left of the decimal
-     |      place. For example, Decimal('123E+1') is converted to Decimal('1.23E+3').
+     |      Convert to an engineering-type string.
+     |
+     |      Engineering notation has an exponent which is a multiple of 3, so there are up
+     |      to 3 digits left of the decimal place. For example, Decimal('123E+1') is
+     |      converted to Decimal('1.23E+3').
      |
      |      The value of context.capitals determines whether the exponent sign is lower
      |      or upper case. Otherwise, the context does not affect the operation.
      |
      |  to_integral(self, /, rounding=None, context=None)
-     |      Identical to the to_integral_value() method.  The to_integral() name has been
-     |      kept for compatibility with older versions.
+     |      Identical to the to_integral_value() method.
+     |
+     |      The to_integral() name has been kept for compatibility with older versions.
      |
      |  to_integral_exact(self, /, rounding=None, context=None)
-     |      Round to the nearest integer, signaling Inexact or Rounded as appropriate if
+     |      Round to the nearest integer.
+     |
+     |      Decimal.to_integral_exact() signals Inexact or Rounded as appropriate if
      |      rounding occurs.  The rounding mode is determined by the rounding parameter
      |      if given, else by the given context. If neither parameter is given, then the
      |      rounding mode of the current default context is used.
      |
      |  to_integral_value(self, /, rounding=None, context=None)
-     |      Round to the nearest integer without signaling Inexact or Rounded.  The
-     |      rounding mode is determined by the rounding parameter if given, else by
-     |      the given context. If neither parameter is given, then the rounding mode
-     |      of the current default context is used.
+     |      Round to the nearest integer without signaling Inexact or Rounded.
+     |
+     |      The rounding mode is determined by the rounding parameter if given, else by
+     |      the given context. If neither parameter is given, then the rounding mode of
+     |      the current default context is used.
      |
      |  ----------------------------------------------------------------------
      |  Class methods defined here:
      |
      |  from_float(f, /)
      |      Class method that converts a float to a decimal number, exactly.
+     |
      |      Since 0.1 is not exactly representable in binary floating point,
      |      Decimal.from_float(0.1) is not the same as Decimal('0.1').
      |

@skirpichev skirpichev marked this pull request as ready for review August 11, 2025 08:06
@skirpichev skirpichev requested a review from AA-Turner August 11, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants