-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-53032: support IEEE 754 contexts in the decimal module #122003
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
Conversation
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.
1be7598
to
b833af8
Compare
@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. |
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No material comments from me - it's simple, and looks good!
CC @vstinner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Misc/NEWS.d/next/Library/2024-07-19-07-16-50.gh-issue-53032.paXN3p.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
For the docs failures: you just need to merge main since we fixed blurb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
I'm worry we only delay the decision to do this. |
We can deprecate it in a follow-up PR. I just wanted to have the changes documented and notified. |
@serhiy-storchaka: So what do you think of this change? Does it look good to you? |
done
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. |
CC @vstinner
@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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a IEEE decimals expert, but technically the code LGTM.
Merged, congrats @skirpichev! |
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:
DECIMAL32
(which is, surprise,32
)ieee_context()
instead)bits>sys.maxsize
?