gh-102431: clarify constraints on arguments of logical_xxx methods

[skirpichev]

• Issue: Documentation of "decimal" library #102431

[skirpichev]

BTW, maybe it does make sense to add is_logical() method (there is _islogical() for pure-Python version) to test constraints for this type of decimal's. Probably, that could simplify documentation, esp. docstrings.

[rhettinger]

I would go farther an include the full text and examples used in Lib/_pydecimal.py:

    def logical_and(self, a, b):
        """Applies the logical operation 'and' between each operand's digits.

        The operands must be both logical numbers.

        >>> ExtendedContext.logical_and(Decimal('0'), Decimal('0'))
        Decimal('0')
        >>> ExtendedContext.logical_and(Decimal('0'), Decimal('1'))
        Decimal('0')
        >>> ExtendedContext.logical_and(Decimal('1'), Decimal('0'))
        Decimal('0')
        >>> ExtendedContext.logical_and(Decimal('1'), Decimal('1'))
        Decimal('1')
        >>> ExtendedContext.logical_and(Decimal('1100'), Decimal('1010'))
        Decimal('1000')
        >>> ExtendedContext.logical_and(Decimal('1111'), Decimal('10'))
        Decimal('10')
        >>> ExtendedContext.logical_and(110, 1101)
        Decimal('100')
        >>> ExtendedContext.logical_and(Decimal(110), 1101)
        Decimal('100')
        >>> ExtendedContext.logical_and(110, Decimal(1101))
        Decimal('100')
        """
        a = _convert_other(a, raiseit=True)
        return a.logical_and(b, context=self)

Anything less that this will just leave the user guessing about how to use these weird functions.

[skirpichev]

I would go farther an include the full text and examples used in Lib/_pydecimal.py:

Agreed. But there should be some formal description too, because "The operands must be both logical numbers." sounds cryptic even after examples... Or do you think it's ok? (The notion of "logical number" is well defined in the sphinx docs.)

I'll sync docstrings for C/Python implementations and add examples.

[skirpichev]

Ok, now docstrings of Context's methods are in sync.
But, maybe we could reduce the number of examples? E.g. in the logical_and case to:

>>> ExtendedContext.logical_and(Decimal('0'), Decimal('0'))
Decimal('0')
>>> ExtendedContext.logical_and(Decimal('0'), Decimal('1'))
Decimal('0')
>>> ExtendedContext.logical_and(Decimal('1'), Decimal('1'))
Decimal('1')
>>> ExtendedContext.logical_and(Decimal('1111'), Decimal('10'))
Decimal('10')

Basically, this shows everything: constraints on digits of operands, how it works digits-wise and how it works with coefficients of different length. I think it should reduce misunderstanding to minimum.

[skirpichev]

@rhettinger, does it make sense for you now?

[skirpichev]

CC @picnixz
CC @serhiy-storchaka

[picnixz]

I think this makes sense to have the inline definition of what a logical operand is but OTOH, one could say that the online docs should be sufficient so I'm neutral on this.

[serhiy-storchaka]

Sorry, I am not a Decimal expert and do not know why such weird operations exist at first place (perhaps it is just the part of some standard).

From practical point of view, I would just refer "logical integers" in the docstrings, and defined them in the module documentation. If you want to add examples in the docstrings and if there are examples in other docstring, I think that a single example per method should be enough. With multiple digits you can cover all cases by a single example. The rest can be added in the module documentation.

[skirpichev]

perhaps it is just the part of some standard

@serhiy-storchaka, sure. If you see something odd in the decimal module - IMO, it's coming from the IBM standard. JFR: https://speleotrove.com/decimal/damisc.html

If you want to add examples in the docstrings and if there are examples in other docstring, I think that a single example per method should be enough.

Current patch just sync Python (which has extensive examples for context functions) and C implementation. Do you think that we should remove (most) examples from pure-Python version?

[serhiy-storchaka]

Well, then we should keep examples. They are not just the documentation, but doctests.

But new additions are incorrect. "Both operands should have zero sign and exponent, and digits either 0 or 1." Operands can be integers, and integers don't have exponent or digits. Lets leave a more detailed explanation to the module documentation.

[skirpichev]

Operands can be integers, and integers don't have exponent or digits.

I doubt we should be more verbose here (docstrings!). And I don't think that sphinx docs needs to be expanded. Context docs already assume that integers are coerced to Decimal's: "Each Context method accepts a Python integer (an instance of int) anywhere that a Decimal instance is accepted."

[serhiy-storchaka]

I also do not think that we should be more verbose here.

[skirpichev]

CC @vstinner

[skirpichev]

PR updated after recent transition of the C decimal module to AC, please review.

Python and C docstrings are in sync. Tiny difference is indentation of examples for C version (for consistency with other docstrings).

Former Python docstrings (not all) have remarks like "The operands must be both logical numbers." We can use that instead of verbose "Both operands should have zero sign and exponent, and digits either 0 or 1." Notion of "logical number" is explained in sphinx docs.

[vstinner]

LGTM

[serhiy-storchaka]

Yes, please keep old remarks.

[vstinner]

LGTM. I'm fine with "The operands must be both logical numbers."

[skirpichev]

@serhiy-storchaka, done

[serhiy-storchaka]

LGTM. 👍

[miss-islington-app]

Thanks @skirpichev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @skirpichev and @serhiy-storchaka, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6ecf77dbdec7838e9ce2298cb8d16e8c2250da81 3.14

[miss-islington-app]

Sorry, @skirpichev and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6ecf77dbdec7838e9ce2298cb8d16e8c2250da81 3.13

[skirpichev]

I'll provide backports.

[bedevere-app]

GH-140105 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-140106 is a backport of this pull request to the 3.13 branch.