Skip to content
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

gh-120010: fix invalid (nan+nanj) results in _Py_c_prod() #120287

Merged
merged 9 commits into from
Dec 6, 2024

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Jun 9, 2024

In some cases, previously computed as (nan+nanj), we could recover meaningful component values in the result, see e.g. the C11, Annex G.5.2, routine _Cmultd():

>>> z = 1e300+1j
>>> z*(nan+infj)  # was (nan+nanj)
(-inf+infj)

That also fix some complex powers for small integer exponents, computed with optimised algorithm (by squaring):

>>> z**5  # was (nan+nanj)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    z**5
    ~^^~
OverflowError: complex exponentiation

In some cases, previously computed as (nan+nanj), we could recover
meaningful component values in the result, see e.g. the C11, Annex
G.5.2, routine _Cmultd():

>>> z = 1e300+1j
>>> z*(nan+infj)  # was (nan+nanj)
(-inf+infj)

That also fix some complex powers for small integer exponents, computed
with optimised algorithm (by squaring):

>>> z**5  # was (nan+nanj)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    z**5
    ~^^~
OverflowError: complex exponentiation
@vstinner
Copy link
Member

cc @mdickinson

@skirpichev skirpichev requested a review from picnixz September 27, 2024 03:44
@skirpichev skirpichev requested a review from picnixz September 27, 2024 10:33
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

@skirpichev skirpichev requested a review from vstinner November 14, 2024 10:40
@vstinner
Copy link
Member

@skirpichev skirpichev requested a review from vstinner

Thanks for this change is beyond my IEEE-754 skills :-( Maybe @serhiy-storchaka can have a look?

@serhiy-storchaka serhiy-storchaka self-requested a review November 18, 2024 16:28
@skirpichev skirpichev marked this pull request as draft November 27, 2024 00:37
@skirpichev
Copy link
Member Author

This should be adjusted after merging #124829.

@skirpichev skirpichev marked this pull request as ready for review November 27, 2024 02:55
@@ -85,11 +85,63 @@ _Py_c_neg(Py_complex a)
}

Py_complex
_Py_c_prod(Py_complex a, Py_complex b)
_Py_c_prod(Py_complex z, Py_complex w)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be ok now. This doesn't require a mixed-mode variant.

Yet, maybe I should change variable names back for consistency with the rest? Current naming here follows to the C standard.

Copy link
Member Author

Choose a reason for hiding this comment

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

@serhiy-storchaka what do you think about renaming of arguments?

I think mapping local variables "abcd" -> "tuvw" will hot hurt readability and then we can preserve original argument names.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with both variants. Preserving names from the old code can make the diff smaller, but in this case the new code is too different from the old code. Using names from the C standard examples helps comparing our code with the original code. The latter looks more important here.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. But this is G.5.1, not G.5.2.

Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka serhiy-storchaka merged commit 8b7c194 into python:main Dec 6, 2024
41 checks passed
@skirpichev skirpichev deleted the cc-in-py_c_prod-120010 branch December 6, 2024 10:28
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…onGH-120287)

In some cases, previously computed as (nan+nanj), we could recover
meaningful component values in the result, see e.g. the C11, Annex
G.5.1, routine _Cmultd():

>>> z = 1e300+1j
>>> z*(nan+infj)  # was (nan+nanj)
(-inf+infj)

That also fix some complex powers for small integer exponents, computed
with optimized algorithm (by squaring):

>>> z**5  # was (nan+nanj)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    z**5
    ~^^~
OverflowError: complex exponentiation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants