FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-53032: support IEEE 754 contexts in the decimal module by skirpichev · Pull Request #122003 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-53032: support IEEE 754 contexts in the decimal module - #122003

Merged
vstinner merged 15 commits into
python:mainfrom
skirpichev:expose-ieee_context-53032
Apr 28, 2025
ConversationCommits (15)ChecksFiles changed
Merged

gh-53032: support IEEE 754 contexts in the decimal module#122003
vstinner merged 15 commits into
python:mainfrom
skirpichev:expose-ieee_context-53032

Conversation

skirpichev commented Jul 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This was in C version from beginning, but available only on conditional compilation (EXTRA_FUNCTIONALITY). Current patch adds function to create IEEE contexts to the pure-python module as well.

Notes for reviewers:

  • this doesn't expose integer constants like DECIMAL32 (which is, surprise, 32)
  • doesn't change function name (I would prefer something like ieee_context() instead)
  • doesn't change error handling. But, perhaps, a ValueError (with same message in all cases) is better than OverflowError if bits>sys.maxsize?

bedevere-app Bot mentioned this pull request Jul 19, 2024
Closed
pythongh-53032: support IEEE 754 contexts in the decimal module
b833af8 
This was in C version from beginning, but available only
on conditional compilation (EXTRA_FUNCTIONALITY).  Current
patch adds function to create IEEE contexts to the
pure-python module as well.
skirpichev force-pushed the expose-ieee_context-53032 branch from 1be7598 to b833af8 Compare July 19, 2024 04:42
skirpichev marked this pull request as ready for review July 19, 2024 05:09
skirpichev requested a review from rhettinger as a code owner July 19, 2024 05:09

rhettinger commented Jul 21, 2024

Copy link
Copy Markdown
Contributor

@tim-one Any thoughts on this? Do anyone need it? Does the name make send? Do we want to go beyond what is in the decimal spec?

I'm neutral on this. It seems both harmless and gratuitous.

skirpichev commented Jul 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

I would expect that any advocacy (or critique) of the feature - belongs rather to the issue thread, not to implementation. BTW, since C23 - these types are part of the C standard.

I should emphasize that even if this will be rejected - there is a bug in main, that should be fixed. See #122081.

rhettinger requested a review from tim-one July 22, 2024 15:06
This was referenced Aug 18, 2024
Merged
Closed
tim-one approved these changes Jan 24, 2025
View reviewed changes

tim-one left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No material comments from me - it's simple, and looks good!

skirpichev commented Jan 24, 2025

Copy link
Copy Markdown
Member Author

CC @vstinner

vstinner approved these changes Jan 24, 2025
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

cc @serhiy-storchaka @picnixz

picnixz reviewed Jan 24, 2025
View reviewed changes
Comment thread Doc/library/decimal.rst
Comment thread Lib/_pydecimal.py Outdated
Comment thread Lib/test/test_decimal.py
Comment thread Lib/test/test_decimal.py Outdated
Comment thread Misc/NEWS.d/next/Library/2024-07-19-07-16-50.gh-issue-53032.paXN3p.rst Outdated
Comment thread Lib/test/test_decimal.py Outdated
Apply suggestions from code review
042d643 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz commented Jan 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

For the docs failures: you just need to merge main since we fixed blurb

serhiy-storchaka reviewed Jan 24, 2025
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since this is a new feature, it needs a What's New entry and the versionadded directive.

I do not know why it was optional and not enabled by default, whether it is correct and what is a use for this feature.

cc @skrah, @mdickinson, @tim-one, @rhettinger

Comment thread Lib/_pydecimal.py Outdated
Comment thread Modules/_decimal/_decimal.c
picnixz approved these changes Jan 27, 2025
View reviewed changes

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm leaving in a few minutes for 2 weeks roughly so just one nit. I haven't looked at the formulation and grammar so a native speaker could check before the merge or we can amend it later if needs arise. Ottherwise I'm happy that we didn't remove the constants even though they could be useless most of the time.

So, if the tests were only moved around and expanded and not reduced, I think this can be merged.

As for why it hasn't been exposed first I think it may have been because it's experimental/wasn't deemed useful at that time maybe?

Comment thread Doc/library/decimal.rst Outdated
skirpichev commented Jan 27, 2025
View reviewed changes
Comment thread Doc/library/decimal.rst Outdated

skirpichev commented Jan 27, 2025

Copy link
Copy Markdown
Member Author

I'm happy that we didn't remove the constants even though they could be useless most of the time.

I'm worry we only delay the decision to do this.

picnixz commented Jan 27, 2025

Copy link
Copy Markdown
Member

We can deprecate it in a follow-up PR. I just wanted to have the changes documented and notified.

vstinner commented Jan 27, 2025

Copy link
Copy Markdown
Member

@serhiy-storchaka: So what do you think of this change? Does it look good to you?

skirpichev commented Feb 15, 2025

Copy link
Copy Markdown
Member Author

What's New entry and the versionadded directive.

done

I do not know why it was optional and not enabled by default, whether it is correct and what is a use for this feature.

It was added to the mpdecimal a long time ago, so I would expect it's correct.

See the issue thread for use cases. BTW, this feature was added in 1919b7e. I would guess it was wrapped under EXTRA_FUNCTIONALITY just because the pure-Python version missed this stuff.

skirpichev commented Apr 28, 2025

Copy link
Copy Markdown
Member Author

CC @vstinner

what is a use for this feature.

@serhiy-storchaka, as Mark said in the issue description - that's to simplify communication with libraries, that understand the IEEE 754 (2008) decimal interchange formats. (This is now a part of C23 as Annex H).

vstinner reviewed Apr 28, 2025
View reviewed changes
Comment thread Lib/_pydecimal.py Outdated
skirpichev commented Apr 28, 2025
View reviewed changes
Comment thread Lib/_pydecimal.py Outdated
serhiy-storchaka approved these changes Apr 28, 2025
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not a IEEE decimals expert, but technically the code LGTM.

vstinner merged commit 5bf0f36 into python:main Apr 28, 2025

vstinner commented Apr 28, 2025

Copy link
Copy Markdown
Member

Merged, congrats @skirpichev!

skirpichev deleted the expose-ieee_context-53032 branch April 28, 2025 13:15
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
serhiy-storchaka serhiy-storchaka approved these changes
picnixz picnixz approved these changes
tim-one tim-one approved these changes
rhettinger Awaiting requested review from rhettinger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL