Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 15 commits intopython:mainfromskirpichev:expose-ieee_context-53032
Apr 28, 2025

Conversation

skirpichev
Copy link
Contributor

@skirpichevskirpichev commentedJul 19, 2024•
edited
Loading

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 likeDECIMAL32 (which is, surprise,32)
  • doesn't change function name (I would prefer something likeieee_context() instead)
  • doesn't change error handling. But, perhaps, a ValueError (with same message in all cases) is better than OverflowError ifbits>sys.maxsize?

This was in C version from beginning, but available onlyon conditional compilation (EXTRA_FUNCTIONALITY).  Currentpatch adds function to create IEEE contexts to thepure-python module as well.
@skirpichevskirpichevforce-pushed theexpose-ieee_context-53032 branch from1be7598 tob833af8CompareJuly 19, 2024 04:42
@skirpichevskirpichev marked this pull request as ready for reviewJuly 19, 2024 05:09
@rhettinger
Copy link
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
Copy link
ContributorAuthor

skirpichev commentedJul 21, 2024•
edited
Loading

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 abug in main, that should be fixed. See#122081.

Copy link
Member

@tim-onetim-one left a 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!

skirpichev reacted with thumbs up emoji
@skirpichev
Copy link
ContributorAuthor

CC@vstinner

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz
Copy link
Member

picnixz commentedJan 24, 2025•
edited
Loading

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

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a 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

Copy link
Member

@picnixzpicnixz left a 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?

@skirpichev
Copy link
ContributorAuthor

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
Copy link
Member

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

@vstinner
Copy link
Member

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

@skirpichev
Copy link
ContributorAuthor

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 in1919b7e. I would guess it was wrapped under EXTRA_FUNCTIONALITY just because the pure-Python version missed this stuff.

@skirpichev
Copy link
ContributorAuthor

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).

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a 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.

@vstinnervstinner merged commit5bf0f36 intopython:mainApr 28, 2025
42 checks passed
@vstinner
Copy link
Member

Merged, congrats@skirpichev!

skirpichev reacted with thumbs up emoji

@skirpichevskirpichev deleted the expose-ieee_context-53032 branchApril 28, 2025 13:15
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@picnixzpicnixzpicnixz approved these changes

@tim-onetim-onetim-one approved these changes

@rhettingerrhettingerAwaiting 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
@skirpichev@rhettinger@picnixz@vstinner@serhiy-storchaka@tim-one
[8]ページ先頭
©2009-2025 Movatter.jp